public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 7/8] Alter table set compression 5+ messages / 4 participants [nested] [flat]
* [PATCH 7/8] Alter table set compression @ 2021-03-10 09:04 Dilip Kumar <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Dilip Kumar @ 2021-03-10 09:04 UTC (permalink / raw) Add support for changing the compression method associated with a column. There are only built-in methods so we don't need to rewrite the table, only the new tuples will be compressed with the new compression method. Dilip Kumar based on the patches from Ildus Kurbangaliev. Design input from Robert Haas and Tomas Vondra. Reviewed by Robert Haas, Tomas Vondra, Alexander Korotkov and Justin Pryzby Discussions: https://www.postgresql.org/message-id/[email protected] https://www.postgresql.org/message-id/CA%2BTgmoaKDW1Oi9V%3Djc9hOGyf77NbkNEABuqgHD1Cq%3D%3D1QsOcxg%40... https://www.postgresql.org/message-id/CA%2BTgmobSDVgUage9qQ5P_%3DF_9jaMkCgyKxUQGtFQU7oN4kX-AA%40mail... https://www.postgresql.org/message-id/20201005160355.byp74sh3ejsv7wrj%40development https://www.postgresql.org/message-id/CAFiTN-tzTTT2oqWdRGLv1dvvS5MC1W%2BLE%2B3bqWPJUZj4GnHOJg%40mail... --- doc/src/sgml/ref/alter_table.sgml | 16 ++ src/backend/commands/tablecmds.c | 198 +++++++++++++++----- src/backend/parser/gram.y | 9 + src/bin/psql/tab-complete.c | 2 +- src/include/nodes/parsenodes.h | 3 +- src/test/regress/expected/compression.out | 47 ++++- src/test/regress/expected/compression_1.out | 48 ++++- src/test/regress/sql/compression.sql | 20 ++ 8 files changed, 296 insertions(+), 47 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 38e416d183..e147831640 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -54,6 +54,7 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET ( <replaceable class="parameter">attribute_option</replaceable> = <replaceable class="parameter">value</replaceable> [, ... ] ) ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> RESET ( <replaceable class="parameter">attribute_option</replaceable> [, ... ] ) ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN } + ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET COMPRESSION <replaceable class="parameter">compression_method</replaceable> ADD <replaceable class="parameter">table_constraint</replaceable> [ NOT VALID ] ADD <replaceable class="parameter">table_constraint_using_index</replaceable> ALTER CONSTRAINT <replaceable class="parameter">constraint_name</replaceable> [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ] @@ -104,6 +105,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( <replaceable>sequence_options</replaceable> ) ] | UNIQUE <replaceable class="parameter">index_parameters</replaceable> | PRIMARY KEY <replaceable class="parameter">index_parameters</replaceable> | + COMPRESSION <replaceable class="parameter">compression_method</replaceable> | REFERENCES <replaceable class="parameter">reftable</replaceable> [ ( <replaceable class="parameter">refcolumn</replaceable> ) ] [ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ] [ ON DELETE <replaceable class="parameter">referential_action</replaceable> ] [ ON UPDATE <replaceable class="parameter">referential_action</replaceable> ] } [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ] @@ -384,6 +386,20 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM </listitem> </varlistentry> + <varlistentry> + <term> + <literal>SET COMPRESSION <replaceable class="parameter">compression_method</replaceable></literal> + </term> + <listitem> + <para> + This sets the compression method for a column. The supported compression + methods are <literal>pglz</literal> and <literal>lz4</literal>. + <literal>lz4</literal> is available only if <literal>--with-lz4</literal> + was used when building <productname>PostgreSQL</productname>. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>ADD <replaceable class="parameter">table_constraint</replaceable> [ NOT VALID ]</literal></term> <listitem> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index a0d2af0cde..bb284741ce 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -532,6 +532,8 @@ static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKM static void ATExecGenericOptions(Relation rel, List *options); static void ATExecSetRowSecurity(Relation rel, bool rls); static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls); +static ObjectAddress ATExecSetCompression(AlteredTableInfo *tab, Relation rel, + const char *column, Node *newValue, LOCKMODE lockmode); static void index_copy_data(Relation rel, RelFileNode newrnode); static const char *storage_name(char c); @@ -3977,6 +3979,7 @@ AlterTableGetLockLevel(List *cmds) */ case AT_GenericOptions: case AT_AlterColumnGenericOptions: + case AT_SetCompression: cmd_lockmode = AccessExclusiveLock; break; @@ -4511,7 +4514,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, case AT_DisableRowSecurity: case AT_ForceRowSecurity: case AT_NoForceRowSecurity: - ATSimplePermissions(rel, ATT_TABLE); + case AT_SetCompression: + ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW); /* These commands never recurse */ /* No command-specific prep needed */ pass = AT_PASS_MISC; @@ -4924,6 +4928,10 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, Assert(rel->rd_rel->relkind == RELKIND_INDEX); ATExecAlterCollationRefreshVersion(rel, cmd->object); break; + case AT_SetCompression: + address = ATExecSetCompression(tab, rel, cmd->name, cmd->def, + lockmode); + break; default: /* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); @@ -7812,6 +7820,67 @@ ATExecSetOptions(Relation rel, const char *colName, Node *options, return address; } +/* + * Helper function for ATExecSetStorage and ATExecSetCompression + * + * Set the attcompression and/or attstorage for the respective index attribute + * if the respective input values are valid. + */ +static void +ApplyChangesToIndexes(Relation rel, Relation attrelation, AttrNumber attnum, + char newcompression, char newstorage, LOCKMODE lockmode) +{ + HeapTuple tuple; + ListCell *lc; + Form_pg_attribute attrtuple; + + foreach(lc, RelationGetIndexList(rel)) + { + Oid indexoid = lfirst_oid(lc); + Relation indrel; + AttrNumber indattnum = 0; + + indrel = index_open(indexoid, lockmode); + + for (int i = 0; i < indrel->rd_index->indnatts; i++) + { + if (indrel->rd_index->indkey.values[i] == attnum) + { + indattnum = i + 1; + break; + } + } + + if (indattnum == 0) + { + index_close(indrel, lockmode); + continue; + } + + tuple = SearchSysCacheCopyAttNum(RelationGetRelid(indrel), indattnum); + + if (HeapTupleIsValid(tuple)) + { + attrtuple = (Form_pg_attribute) GETSTRUCT(tuple); + + if (CompressionMethodIsValid(newcompression)) + attrtuple->attcompression = newcompression; + + if (newstorage != '\0') + attrtuple->attstorage = newstorage; + + CatalogTupleUpdate(attrelation, &tuple->t_self, tuple); + + InvokeObjectPostAlterHook(RelationRelationId, + RelationGetRelid(rel), + attrtuple->attnum); + + heap_freetuple(tuple); + } + + index_close(indrel, lockmode); + } +} /* * ALTER TABLE ALTER COLUMN SET STORAGE * @@ -7827,7 +7896,6 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc Form_pg_attribute attrtuple; AttrNumber attnum; ObjectAddress address; - ListCell *lc; Assert(IsA(newValue, String)); storagemode = strVal(newValue); @@ -7891,47 +7959,8 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc * Apply the change to indexes as well (only for simple index columns, * matching behavior of index.c ConstructTupleDescriptor()). */ - foreach(lc, RelationGetIndexList(rel)) - { - Oid indexoid = lfirst_oid(lc); - Relation indrel; - AttrNumber indattnum = 0; - - indrel = index_open(indexoid, lockmode); - - for (int i = 0; i < indrel->rd_index->indnatts; i++) - { - if (indrel->rd_index->indkey.values[i] == attnum) - { - indattnum = i + 1; - break; - } - } - - if (indattnum == 0) - { - index_close(indrel, lockmode); - continue; - } - - tuple = SearchSysCacheCopyAttNum(RelationGetRelid(indrel), indattnum); - - if (HeapTupleIsValid(tuple)) - { - attrtuple = (Form_pg_attribute) GETSTRUCT(tuple); - attrtuple->attstorage = newstorage; - - CatalogTupleUpdate(attrelation, &tuple->t_self, tuple); - - InvokeObjectPostAlterHook(RelationRelationId, - RelationGetRelid(rel), - attrtuple->attnum); - - heap_freetuple(tuple); - } - - index_close(indrel, lockmode); - } + ApplyChangesToIndexes(rel, attrelation, attnum, InvalidCompressionMethod, + newstorage, lockmode); table_close(attrelation, RowExclusiveLock); @@ -15118,6 +15147,89 @@ ATExecGenericOptions(Relation rel, List *options) heap_freetuple(tuple); } +/* + * ALTER TABLE ALTER COLUMN SET COMPRESSION + * + * Return value is the address of the modified column + */ +static ObjectAddress +ATExecSetCompression(AlteredTableInfo *tab, + Relation rel, + const char *column, + Node *newValue, + LOCKMODE lockmode) +{ + Relation attrel; + HeapTuple tuple; + Form_pg_attribute atttableform; + AttrNumber attnum; + char *compression; + char typstorage; + Oid cmoid; + Datum values[Natts_pg_attribute]; + bool nulls[Natts_pg_attribute]; + bool replace[Natts_pg_attribute]; + ObjectAddress address; + + Assert(IsA(newValue, String)); + compression = strVal(newValue); + + attrel = table_open(AttributeRelationId, RowExclusiveLock); + + tuple = SearchSysCacheAttName(RelationGetRelid(rel), column); + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_COLUMN), + errmsg("column \"%s\" of relation \"%s\" does not exist", + column, RelationGetRelationName(rel)))); + + /* prevent them from altering a system attribute */ + atttableform = (Form_pg_attribute) GETSTRUCT(tuple); + attnum = atttableform->attnum; + if (attnum <= 0) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot alter system column \"%s\"", column))); + + typstorage = get_typstorage(atttableform->atttypid); + + /* prevent from setting compression methods for uncompressible type */ + if (!IsStorageCompressible(typstorage)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("column data type %s does not support compression", + format_type_be(atttableform->atttypid)))); + + /* initialize buffers for new tuple values */ + memset(values, 0, sizeof(values)); + memset(nulls, false, sizeof(nulls)); + memset(replace, false, sizeof(replace)); + + /* get the attribute compression method. */ + cmoid = GetAttributeCompression(atttableform, compression); + + atttableform->attcompression = cmoid; + CatalogTupleUpdate(attrel, &tuple->t_self, tuple); + + InvokeObjectPostAlterHook(RelationRelationId, + RelationGetRelid(rel), + atttableform->attnum); + + ReleaseSysCache(tuple); + + /* apply changes to the index column as well */ + ApplyChangesToIndexes(rel, attrel, attnum, cmoid, '\0', lockmode); + table_close(attrel, RowExclusiveLock); + + /* make changes visible */ + CommandCounterIncrement(); + + ObjectAddressSubSet(address, RelationRelationId, + RelationGetRelid(rel), atttableform->attnum); + return address; +} + + /* * Preparation phase for SET LOGGED/UNLOGGED * diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index f8bc11d28b..07aa3c9330 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -2308,6 +2308,15 @@ alter_table_cmd: n->missing_ok = true; $$ = (Node *)n; } + /* ALTER TABLE <name> ALTER [COLUMN] <colname> SET (COMPRESSION <cm>) */ + | ALTER opt_column ColId SET optColumnCompression + { + AlterTableCmd *n = makeNode(AlterTableCmd); + n->subtype = AT_SetCompression; + n->name = $3; + n->def = (Node *) makeString($5); + $$ = (Node *)n; + } /* ALTER TABLE <name> DROP [COLUMN] IF EXISTS <colname> [RESTRICT|CASCADE] */ | DROP opt_column IF_P EXISTS ColId opt_drop_behavior { diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index fdd1bdd92a..ebe07a17cd 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2115,7 +2115,7 @@ psql_completion(const char *text, int start, int end) /* ALTER TABLE ALTER [COLUMN] <foo> SET */ else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny, "SET") || Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny, "SET")) - COMPLETE_WITH("(", "DEFAULT", "NOT NULL", "STATISTICS", "STORAGE"); + COMPLETE_WITH("(", "COMPRESSION", "DEFAULT", "NOT NULL", "STATISTICS", "STORAGE"); /* ALTER TABLE ALTER [COLUMN] <foo> SET ( */ else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny, "SET", "(") || Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny, "SET", "(")) diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 56e24529f1..b20c03f691 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1902,7 +1902,8 @@ typedef enum AlterTableType AT_AddIdentity, /* ADD IDENTITY */ AT_SetIdentity, /* SET identity column options */ AT_DropIdentity, /* DROP IDENTITY */ - AT_AlterCollationRefreshVersion /* ALTER COLLATION ... REFRESH VERSION */ + AT_AlterCollationRefreshVersion, /* ALTER COLLATION ... REFRESH VERSION */ + AT_SetCompression /* SET COMPRESSION */ } AlterTableType; typedef struct ReplicaIdentityStmt diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out index 9ddffc8b18..19625708a9 100644 --- a/src/test/regress/expected/compression.out +++ b/src/test/regress/expected/compression.out @@ -247,12 +247,57 @@ CREATE TABLE cmdata2 (f1 text); --------+------+-----------+----------+---------+----------+-------------+--------------+------------- f1 | text | | | | extended | lz4 | | +-- test alter compression method +ALTER TABLE cmdata ALTER COLUMN f1 SET COMPRESSION lz4; +INSERT INTO cmdata VALUES (repeat('123456789',4004)); +\d+ cmdata + Table "public.cmdata" + Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +--------+------+-----------+----------+---------+----------+-------------+--------------+------------- + f1 | text | | | | extended | lz4 | | +Indexes: + "idx" btree (f1) + +SELECT pg_column_compression(f1) FROM cmdata; + pg_column_compression +----------------------- + pglz + lz4 +(2 rows) + +-- test alter compression method for the materialized view +ALTER MATERIALIZED VIEW mv ALTER COLUMN x SET COMPRESSION lz4; +\d+ mv + Materialized view "public.mv" + Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +--------+------+-----------+----------+---------+----------+-------------+--------------+------------- + x | text | | | | extended | lz4 | | +View definition: + SELECT cmdata1.f1 AS x + FROM cmdata1; + +-- test alter compression method for the partitioned table +ALTER TABLE cmpart1 ALTER COLUMN f1 SET COMPRESSION pglz; +ALTER TABLE cmpart2 ALTER COLUMN f1 SET COMPRESSION lz4; +-- new data should be compressed with the current compression method +INSERT INTO cmpart VALUES (repeat('123456789',1004)); +INSERT INTO cmpart VALUES (repeat('123456789',4004)); +SELECT pg_column_compression(f1) FROM cmpart; + pg_column_compression +----------------------- + lz4 + pglz + pglz + lz4 +(4 rows) + -- check data is ok SELECT length(f1) FROM cmdata; length -------- 10000 -(1 row) + 36036 +(2 rows) SELECT length(f1) FROM cmdata1; length diff --git a/src/test/regress/expected/compression_1.out b/src/test/regress/expected/compression_1.out index 03a7f46554..bd642634a6 100644 --- a/src/test/regress/expected/compression_1.out +++ b/src/test/regress/expected/compression_1.out @@ -256,12 +256,58 @@ CREATE TABLE cmdata2 (f1 text); --------+------+-----------+----------+---------+----------+-------------+--------------+------------- f1 | text | | | | extended | pglz | | +-- test alter compression method +ALTER TABLE cmdata ALTER COLUMN f1 SET COMPRESSION lz4; +ERROR: unsupported LZ4 compression method +DETAIL: This functionality requires the server to be built with lz4 support. +HINT: You need to rebuild PostgreSQL using --with-lz4. +INSERT INTO cmdata VALUES (repeat('123456789',4004)); +\d+ cmdata + Table "public.cmdata" + Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +--------+------+-----------+----------+---------+----------+-------------+--------------+------------- + f1 | text | | | | extended | pglz | | +Indexes: + "idx" btree (f1) + +SELECT pg_column_compression(f1) FROM cmdata; + pg_column_compression +----------------------- + pglz + pglz +(2 rows) + +-- test alter compression method for the materialized view +ALTER MATERIALIZED VIEW mv ALTER COLUMN x SET COMPRESSION lz4; +ERROR: relation "mv" does not exist +\d+ mv +-- test alter compression method for the partitioned table +ALTER TABLE cmpart1 ALTER COLUMN f1 SET COMPRESSION pglz; +ERROR: relation "cmpart1" does not exist +ALTER TABLE cmpart2 ALTER COLUMN f1 SET COMPRESSION lz4; +ERROR: unsupported LZ4 compression method +DETAIL: This functionality requires the server to be built with lz4 support. +HINT: You need to rebuild PostgreSQL using --with-lz4. +-- new data should be compressed with the current compression method +INSERT INTO cmpart VALUES (repeat('123456789',1004)); +ERROR: relation "cmpart" does not exist +LINE 1: INSERT INTO cmpart VALUES (repeat('123456789',1004)); + ^ +INSERT INTO cmpart VALUES (repeat('123456789',4004)); +ERROR: relation "cmpart" does not exist +LINE 1: INSERT INTO cmpart VALUES (repeat('123456789',4004)); + ^ +SELECT pg_column_compression(f1) FROM cmpart; +ERROR: relation "cmpart" does not exist +LINE 1: SELECT pg_column_compression(f1) FROM cmpart; + ^ -- check data is ok SELECT length(f1) FROM cmdata; length -------- 10000 -(1 row) + 36036 +(2 rows) SELECT length(f1) FROM cmdata1; ERROR: relation "cmdata1" does not exist diff --git a/src/test/regress/sql/compression.sql b/src/test/regress/sql/compression.sql index 6c9b24f1f7..00b1ff1137 100644 --- a/src/test/regress/sql/compression.sql +++ b/src/test/regress/sql/compression.sql @@ -111,6 +111,26 @@ DROP TABLE cmdata2; CREATE TABLE cmdata2 (f1 text); \d+ cmdata2 +-- test alter compression method +ALTER TABLE cmdata ALTER COLUMN f1 SET COMPRESSION lz4; +INSERT INTO cmdata VALUES (repeat('123456789',4004)); +\d+ cmdata +SELECT pg_column_compression(f1) FROM cmdata; + +-- test alter compression method for the materialized view +ALTER MATERIALIZED VIEW mv ALTER COLUMN x SET COMPRESSION lz4; +\d+ mv + +-- test alter compression method for the partitioned table +ALTER TABLE cmpart1 ALTER COLUMN f1 SET COMPRESSION pglz; +ALTER TABLE cmpart2 ALTER COLUMN f1 SET COMPRESSION lz4; + +-- new data should be compressed with the current compression method +INSERT INTO cmpart VALUES (repeat('123456789',1004)); +INSERT INTO cmpart VALUES (repeat('123456789',4004)); + +SELECT pg_column_compression(f1) FROM cmpart; + -- check data is ok SELECT length(f1) FROM cmdata; SELECT length(f1) FROM cmdata1; -- 2.17.0 --cvVnyQ+4j833TQvp Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0008-default-to-with-lz4.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: run pgindent on a regular basis / scripted manner @ 2023-01-22 22:38 Andres Freund <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Andres Freund @ 2023-01-22 22:38 UTC (permalink / raw) To: Jelte Fennema <[email protected]>; +Cc: Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>; Bruce Momjian <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; pgsql-hackers Hi, On 2023-01-22 18:20:49 +0100, Jelte Fennema wrote: > > But switching away from that intermixed with a lot of other changes isn't going to be fun. > > I don't think the amount of pain is really much lower if we reformat > 10,000 or 300,000 lines of code, without automation both would be > quite painful. But the git commands I shared in my previous email > should alleviate most of that pain. It's practically not possible to review a 300k line change. And perhaps I'm paranoid, but I would have a problem with a commit in the history that's practically not reviewable. > > I don't have a problem with the current pgindent alignment of function > > parameters, so not sure what you mean about that. > > Function parameter alignment is fine with pgindent imho, but this +12 > column variable declaration thing I personally think is quite weird. I strongly dislike it, I rarely get it right by hand - but it does have some benefit over aligning variable names based on the length of the type names as uncrustify/clang-format: In their approach an added local variable can cause all the other variables to be re-indented (and their initial value possibly wrapped). The fixed alignment doesn't have that issue. Personally I think the cost of trying to align local variable names is way way higher than the benefit. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: run pgindent on a regular basis / scripted manner @ 2023-01-22 22:47 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: Tom Lane @ 2023-01-22 22:47 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Jelte Fennema <[email protected]>; Peter Geoghegan <[email protected]>; Bruce Momjian <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: > I strongly dislike it, I rarely get it right by hand - but it does have some > benefit over aligning variable names based on the length of the type names as > uncrustify/clang-format: In their approach an added local variable can cause > all the other variables to be re-indented (and their initial value possibly > wrapped). The fixed alignment doesn't have that issue. Yeah. That's one of my biggest gripes about pgperltidy: if you insert another assignment in a series of assignments, it is very likely to reformat all the adjacent assignments because it thinks it's cool to make all the equal signs line up. That's just awful. You can either run pgperltidy on new code before committing, and accept that the feature patch will touch a lot of lines it's not making real changes to (thereby dirtying the "git blame" history) or not do so and thereby commit code that's not passing tidiness checks. Let's *not* adopt any style that causes similar things to start happening in our C code. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: run pgindent on a regular basis / scripted manner @ 2023-01-22 23:28 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Tom Lane @ 2023-01-22 23:28 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Jelte Fennema <[email protected]>; Peter Geoghegan <[email protected]>; Bruce Momjian <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: > On 2023-01-22 18:20:49 +0100, Jelte Fennema wrote: >> I don't think the amount of pain is really much lower if we reformat >> 10,000 or 300,000 lines of code, without automation both would be >> quite painful. But the git commands I shared in my previous email >> should alleviate most of that pain. > It's practically not possible to review a 300k line change. And perhaps I'm > paranoid, but I would have a problem with a commit in the history that's > practically not reviewable. As far as that goes, if you had concern then you could run the indentation tool locally and confirm you got matching results. But this does point up that the processes Jelte suggested all depend critically on indentation results being 100% reproducible by anybody. So the more I think about it the less excited I am about depending on clang-format, because version skew in peoples' clang installations seems inevitable, and there's good reason to fear that that would show up as varying indentation results. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: run pgindent on a regular basis / scripted manner @ 2023-02-03 16:44 Andrew Dunstan <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Andrew Dunstan @ 2023-02-03 16:44 UTC (permalink / raw) To: Tom Lane <[email protected]>; Andres Freund <[email protected]>; +Cc: Jelte Fennema <[email protected]>; Peter Geoghegan <[email protected]>; Bruce Momjian <[email protected]>; Magnus Hagander <[email protected]>; Alvaro Herrera <[email protected]>; Stephen Frost <[email protected]>; Noah Misch <[email protected]>; Jesse Zhang <[email protected]>; pgsql-hackers On 2023-01-22 Su 17:47, Tom Lane wrote: > Andres Freund<[email protected]> writes: >> I strongly dislike it, I rarely get it right by hand - but it does have some >> benefit over aligning variable names based on the length of the type names as >> uncrustify/clang-format: In their approach an added local variable can cause >> all the other variables to be re-indented (and their initial value possibly >> wrapped). The fixed alignment doesn't have that issue. > Yeah. That's one of my biggest gripes about pgperltidy: if you insert > another assignment in a series of assignments, it is very likely to > reformat all the adjacent assignments because it thinks it's cool to > make all the equal signs line up. That's just awful. You can either > run pgperltidy on new code before committing, and accept that the feature > patch will touch a lot of lines it's not making real changes to (thereby > dirtying the "git blame" history) or not do so and thereby commit code > that's not passing tidiness checks. Let's *not* adopt any style that > causes similar things to start happening in our C code. Modern versions of perltidy give you much more control over this, so maybe we need to investigate the possibility of updating. See the latest docco at <*https://metacpan.org/dist/Perl-Tidy/view/bin/perltidy#Completely-turning-off-vertical-alignment-with...; * Probably we'd want to use something like |--valign-exclusion-list=||'= => ,'| || || |cheers| || |andrew| -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-02-03 16:44 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-10 09:04 [PATCH 7/8] Alter table set compression Dilip Kumar <[email protected]> 2023-01-22 22:38 Re: run pgindent on a regular basis / scripted manner Andres Freund <[email protected]> 2023-01-22 22:47 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]> 2023-02-03 16:44 ` Re: run pgindent on a regular basis / scripted manner Andrew Dunstan <[email protected]> 2023-01-22 23:28 ` Re: run pgindent on a regular basis / scripted manner Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox