Repository: dbt-labs/dbt-core
Pull request: Fix postgres handling for unlimited varchars
resolves #5238 

### Description

Handles `varchar` the same way as `text` for postgres columns (since unlimited `varchar` on postgres is not implicitely limited to 256 characters as on redshift)

### Checklist

- [x] I have read [the contributing guide](https://github.com/dbt-labs/dbt-core/blob/main/CONTRIBUTING.md) and understand what's expected of me
- [x] I have signed the [CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [x] I have run this code in development and it appears to resolve the stated issue
- [x] This PR includes tests, or tests are not required/relevant for this PR
- [x] I have [opened an issue to add/update docs](https://github.com/dbt-labs/docs.getdbt.com/issues/new/choose), or docs changes are not required/relevant for this PR
- [x] I have run `changie new` to [create a changelog entry](https://github.com/dbt-labs/dbt-core/blob/main/CONTRIBUTING.md#Adding-CHANGELOG-Entry)


Problem statement:
[CT-636] [Bug] Postgres unlimited varchar default to varchar(256)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current Behavior

When using postgres adapter with some varchar typed columns (like when using dbt_utils.type_string()), if you use incremental materialization with sync_new_columns or append_new_columns, or actually anything that will make dbt generate DDL statements, varchars max size defaults to 256 (handler in base/Column.py behaviour when char_size is None). This makes schema evolution impossible when having textual columns defined as {{ dbt_utils.type_string() }}

### Expected Behavior

The original varchar type with no limit should remain as is and not be assigned a 256 default limit, because we don't know the size limit on a varchar without explicit limits. The schema evolution should be possible, with new columns not created as varchar(256)

### Steps To Reproduce

1. Create an incremental model with on_schema_change: sync_all_columns
2. run the model for the first time
3. add a column whose type is {{ dbt_utils.type_string() }} and whose length exceeds 256 characters
4. re-run the model. It will fail because it adds a varchar(256) column

### Relevant log output

```shell
Debug logs for the schema update : 
 "database"."schema"."table":
         Schema changed: True
         Source columns not in target: [<Column col1 (character varying(256))>, <Column col2 (character varying(256))>, <Column col3 (character varying(256))>]
         Target columns not in source: [<Column col4 (character varying(256))>]
         New column types: []
(Note that all these columns are typed are varchar initially not varchar(256))
Postgres columns data check : 
```
```sql
select
          column_name,
          data_type,
          character_maximum_length,
          numeric_precision,
          numeric_scale

      from "production-data-warehouse".INFORMATION_SCHEMA.columns
      where table_name = '{{table}}'
        
        and table_schema = '{{schema}}'
        
      order by ordinal_position
```
```shell
Error: 
`value too long for type character varying(256)`
```


### Environment

```markdown
- OS: Any
- Python: 3.9-slim
- dbt: 1.0.0
```


### What database are you using dbt with?

postgres

### Additional Context

I already found out why it worked like that, and if validated will be happy to submit a PR (linked to the fact that the way postgres columns are retrieved have NULL info regarding varchar columns max_length, and thus when using it it falls back in default Column handler)
Just wondered if this shall be fixed in the PostgresColumn class (that already handles text columns this way), or if we shall just update the dbt_utils.string_type macro to use text instead of varchar for postgres

Hints from issue discussion:
> Just wondered if this shall be fixed in the PostgresColumn class (that already handles text columns this way), or if we shall just update the dbt_utils.string_type macro to use text instead of varchar for postgres

Yes! Very relevant: https://github.com/dbt-labs/dbt-utils/pull/586
Seems really nice ! 
If i understand correctly, that PR means that it will shift from varchar to text for Postgres + Snowflake right ? But isn't it safer to still handle the varchar without limit in the PostgresColumn, tu avoid issues when explicitely using this type outside type macros ? 
Ah, I see what you mean! In this logic:

https://github.com/dbt-labs/dbt-core/blob/0d8e061a3d8d726bb10ff767d8f276439cc472b2/core/dbt/adapters/base/column.py#L85-L93

You're saying, on Postgres, it actually _should_ be allowed to remain `None`—or really `65535` (?), since we may want a not-none value here to support the comparison in [`can_expand_to`](https://github.com/dbt-labs/dbt-core/blob/0d8e061a3d8d726bb10ff767d8f276439cc472b2/core/dbt/adapters/base/column.py#L101). Does that sound right?

I think this may also be a place where the behavior differs between Postgres + Redshift. [On Redshift](https://docs.aws.amazon.com/redshift/latest/dg/r_Character_types.html), `TEXT` is just an alias for `VARCHAR(256)`, and:

> If you use the VARCHAR data type without a length specifier in a CREATE TABLE statement, the default length is 256. If used in an expression, the size of the output is determined using the input expression (up to 65535).

It's totally appropriate to implement different logic for `dbt-postgres` and `dbt-redshift` accordingly. Just noting it here because both currently use the base `Column` implementation.
As I could see Postgres already have a different way of handling TEXT columns and supersedes its parent in this case here : 
https://github.com/dbt-labs/dbt-core/blob/0d8e061a3d8d726bb10ff767d8f276439cc472b2/plugins/postgres/dbt/adapters/postgres/relation.py#L26-L32

Also, for postgres, max varchar size is 10485760 as stated [here](https://dba.stackexchange.com/questions/189876/size-limit-of-character-varying-postgresql), but only when explicitely stated (if not, it's actually the limit for any column of 1Gb).

My guess is that it would be okay / accurate to also handle unlimited varchar the same way TEXT is handled in postgres, since i don't think the 256 default for postgres is the intended behaviour

I'll be glad to submit a pr for this point if it seems legitimate
You're so right!! Sorry I missed that. I think adding a new method to `PostgresColumn` is the way to go, and I'd welcome a PR for it

Commit message: chore: added changelog entry

Patch to review:
diff --git a/.changes/unreleased/Fixes-20220523-103843.yaml b/.changes/unreleased/Fixes-20220523-103843.yaml
new file mode 100644
index 00000000000..1235865763c
--- /dev/null
+++ b/.changes/unreleased/Fixes-20220523-103843.yaml
@@ -0,0 +1,8 @@
+kind: Fixes
+body: Remove the default 256 characters limit on postgres character varying type when
+  no limitation is set
+time: 2022-05-23T10:38:43.392232+02:00
+custom:
+  Author: shrodingers
+  Issue: "636"
+  PR: "5292"
diff --git a/plugins/postgres/dbt/adapters/postgres/relation.py b/plugins/postgres/dbt/adapters/postgres/relation.py
index 517984c281a..0f3296c1818 100644
--- a/plugins/postgres/dbt/adapters/postgres/relation.py
+++ b/plugins/postgres/dbt/adapters/postgres/relation.py
@@ -26,7 +26,9 @@ def relation_max_name_length(self):
 class PostgresColumn(Column):
     @property
     def data_type(self):
-        # on postgres, do not convert 'text' to 'varchar()'
-        if self.dtype.lower() == "text":
+        # on postgres, do not convert 'text' or 'varchar' to 'varchar()'
+        if self.dtype.lower() == "text" or (
+            self.dtype.lower() == "character varying" and self.char_size is None
+        ):
             return self.dtype
         return super().data_type


Code review comments:
diff_hunk: @@ -0,0 +1,8 @@
+kind: Fixes
+body: Remove the default 256 characters limit on postgres character varying type when
+  no limitation is set
+time: 2022-05-23T10:38:43.392232+02:00
+custom:
+  Author: shrodingers
+  Issue: "636"
path: .changes/unreleased/Fixes-20220523-103843.yaml
text: @user1:
```suggestion
  Issue: "5238"
```
This should be the GitHub issue number.  636 is the issue number in our Jira instance.