Re: tablecmds.c/MergeAttributes() cleanup
On Fri, May 3, 2024 at 2:47 PM Peter Eisentraut wrote: > On 30.04.24 21:48, Robert Haas wrote: > > I took a look at this patch. Currently this case crashes: > > > > CREATE TABLE cmdata(f1 text COMPRESSION pglz); > > CREATE TABLE cmdata3(f1 text); > > CREATE TABLE cminh() INHERITS (cmdata, cmdata3); > > > > The patch makes this succeed, but I was initially unclear why it > > didn't make it fail with an error instead: you can argue that cmdata > > has pglz and cmdata3 has default and those are different. It seems > > that prior precedent goes both ways -- we treat the absence of a > > STORAGE specification as STORAGE EXTENDED and it conflicts with an > > explicit storage specification on some other inheritance parent - but > > on the other hand, we treat the absence of a default as compatible > > with any explicit default, similar to what happens here. > > The actual behavior here is arguably not ideal. It was the purpose of > the other thread mentioned upthread to improve that, but that was not > successful for the time being. > > > So now I think this is committable, but I can't do it now because I > > won't be around for the next few hours in case the buildfarm blows up. > > I can do it tomorrow, or perhaps Peter would like to handle it since > > it seems to have been his commit that introduced the issue. > > I have committed it now. > > Thanks Peter. -- Best Wishes, Ashutosh Bapat
Re: tablecmds.c/MergeAttributes() cleanup
On 30.04.24 21:48, Robert Haas wrote: I took a look at this patch. Currently this case crashes: CREATE TABLE cmdata(f1 text COMPRESSION pglz); CREATE TABLE cmdata3(f1 text); CREATE TABLE cminh() INHERITS (cmdata, cmdata3); The patch makes this succeed, but I was initially unclear why it didn't make it fail with an error instead: you can argue that cmdata has pglz and cmdata3 has default and those are different. It seems that prior precedent goes both ways -- we treat the absence of a STORAGE specification as STORAGE EXTENDED and it conflicts with an explicit storage specification on some other inheritance parent - but on the other hand, we treat the absence of a default as compatible with any explicit default, similar to what happens here. The actual behavior here is arguably not ideal. It was the purpose of the other thread mentioned upthread to improve that, but that was not successful for the time being. So now I think this is committable, but I can't do it now because I won't be around for the next few hours in case the buildfarm blows up. I can do it tomorrow, or perhaps Peter would like to handle it since it seems to have been his commit that introduced the issue. I have committed it now.
Re: tablecmds.c/MergeAttributes() cleanup
On Tue, Apr 30, 2024 at 2:19 AM Ashutosh Bapat wrote: > On Mon, Apr 29, 2024 at 6:46 PM Robert Haas wrote: >> On Sat, Apr 20, 2024 at 12:17 AM Ashutosh Bapat >> wrote: >> > Yes please. Probably this issue surfaced again after we reverted >> > compression and storage fix? Please If that's the case, please add it to >> > the open items. >> >> This is still on the open items list and I'm not clear who, if anyone, >> is working on fixing it. >> >> It would be good if someone fixed it. :-) > > Here's a patch fixing it. > > I have added the reproducer provided by Alexander as a test. I thought of > improving that test further to test the compression of the inherited table > but did not implement it since we haven't documented the behaviour of > compression with inheritance. Defining and implementing compression behaviour > for inherited tables was the goal of > 0413a556990ba628a3de8a0b58be020fd9a14ed0, which has been reverted. I took a look at this patch. Currently this case crashes: CREATE TABLE cmdata(f1 text COMPRESSION pglz); CREATE TABLE cmdata3(f1 text); CREATE TABLE cminh() INHERITS (cmdata, cmdata3); The patch makes this succeed, but I was initially unclear why it didn't make it fail with an error instead: you can argue that cmdata has pglz and cmdata3 has default and those are different. It seems that prior precedent goes both ways -- we treat the absence of a STORAGE specification as STORAGE EXTENDED and it conflicts with an explicit storage specification on some other inheritance parent - but on the other hand, we treat the absence of a default as compatible with any explicit default, similar to what happens here. But I eventually realized that you're just putting back behavior that we had in previous releases: pre-v17, the code already works the way this patch makes it do, and MergeChildAttribute() is already coded similar to this. As Alexander Lakhin said upthread, 4d969b2f8 seems to have broken this. So now I think this is committable, but I can't do it now because I won't be around for the next few hours in case the buildfarm blows up. I can do it tomorrow, or perhaps Peter would like to handle it since it seems to have been his commit that introduced the issue. -- Robert Haas EDB: http://www.enterprisedb.com
Re: tablecmds.c/MergeAttributes() cleanup
On Mon, Apr 29, 2024 at 6:46 PM Robert Haas wrote:
> On Sat, Apr 20, 2024 at 12:17 AM Ashutosh Bapat
> wrote:
> > Yes please. Probably this issue surfaced again after we reverted
> compression and storage fix? Please If that's the case, please add it to
> the open items.
>
> This is still on the open items list and I'm not clear who, if anyone,
> is working on fixing it.
>
> It would be good if someone fixed it. :-)
>
Here's a patch fixing it.
I have added the reproducer provided by Alexander as a test. I thought of
improving that test further to test the compression of the inherited table
but did not implement it since we haven't documented the behaviour of
compression with inheritance. Defining and implementing compression
behaviour for inherited tables was the goal
of 0413a556990ba628a3de8a0b58be020fd9a14ed0, which has been reverted.
--
Best Wishes,
Ashutosh Bapat
From 7c1ff7b17933eef9523486a2c0a054836db9cf24 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat
Date: Tue, 30 Apr 2024 11:19:43 +0530
Subject: [PATCH] Fix segmentation fault in MergeInheritedAttribute()
While converting a pg_attribute tuple into a ColumnDef, ColumnDef::compression
remains NULL if there is no compression method set fot the attribute. Calling
strcmp() with NULL ColumnDef::compression, when comparing compression methods
of parents, causes segmentation fault in MergeInheritedAttribute(). Skip
comparing compression methods if either of them is NULL.
Author: Ashutosh Bapat
Discussion: https://www.postgresql.org/message-id/b22a6834-aacb-7b18-0424-a3f5fe889667%40gmail.com
---
src/backend/commands/tablecmds.c | 16 ++--
src/test/regress/expected/compression.out | 10 +++---
src/test/regress/sql/compression.sql | 8 +---
3 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3556240c8e..e29f96e357 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3430,12 +3430,16 @@ MergeInheritedAttribute(List *inh_columns,
*/
if (prevdef->compression == NULL)
prevdef->compression = newdef->compression;
- else if (strcmp(prevdef->compression, newdef->compression) != 0)
- ereport(ERROR,
-(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("column \"%s\" has a compression method conflict",
- attributeName),
- errdetail("%s versus %s", prevdef->compression, newdef->compression)));
+ else if (newdef->compression != NULL)
+ {
+ if (strcmp(prevdef->compression, newdef->compression) != 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("column \"%s\" has a compression method conflict",
+ attributeName),
+ errdetail("%s versus %s",
+ prevdef->compression, newdef->compression)));
+ }
/*
* Check for GENERATED conflicts
diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out
index 834b7555cb..4dd9ee7200 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -223,15 +223,18 @@ SELECT pg_column_compression(f1) FROM cmpart2;
pglz
(1 row)
--- test compression with inheritance, error
-CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
+-- test compression with inheritance
+CREATE TABLE cminh() INHERITS(cmdata, cmdata1); -- error
NOTICE: merging multiple inherited definitions of column "f1"
ERROR: column "f1" has a compression method conflict
DETAIL: pglz versus lz4
-CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
+CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata); -- error
NOTICE: merging column "f1" with inherited definition
ERROR: column "f1" has a compression method conflict
DETAIL: pglz versus lz4
+CREATE TABLE cmdata3(f1 text);
+CREATE TABLE cminh() INHERITS (cmdata, cmdata3);
+NOTICE: merging multiple inherited definitions of column "f1"
-- test default_toast_compression GUC
SET default_toast_compression = '';
ERROR: invalid value for parameter "default_toast_compression": ""
@@ -251,6 +254,7 @@ INSERT INTO cmdata VALUES (repeat('123456789', 4004));
f1 | text | | | | extended | lz4 | |
Indexes:
"idx" btree (f1)
+Child tables: cminh
SELECT pg_column_compression(f1) FROM cmdata;
pg_column_compression
diff --git a/src/test/regress/sql/compression.sql b/src/test/regress/sql/compression.sql
index 7179a5002e..490595fcfb 100644
--- a/src/test/regress/sql/compression.sql
+++ b/src/test/regress/sql/compression.sql
@@ -93,9 +93,11 @@ INSERT INTO cmpart VALUES (repeat('123456789', 4004));
SELECT pg_column_compression(f1) FROM cmpart1;
SELECT pg_column_compression(f1) FROM cmpart2;
--- test compression with inheritance, error
-CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
-CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
+-- test compression with inheritance
+CREATE TABLE cminh() INHERITS(cmdata, cmdata1); -- error
Re: tablecmds.c/MergeAttributes() cleanup
On Sat, Apr 20, 2024 at 12:17 AM Ashutosh Bapat wrote: > Yes please. Probably this issue surfaced again after we reverted compression > and storage fix? Please If that's the case, please add it to the open items. This is still on the open items list and I'm not clear who, if anyone, is working on fixing it. It would be good if someone fixed it. :-) -- Robert Haas EDB: http://www.enterprisedb.com
Re: tablecmds.c/MergeAttributes() cleanup
On Sat, Apr 20, 2024 at 9:30 AM Alexander Lakhin wrote: > Hello, > > 30.01.2024 09:22, Ashutosh Bapat wrote: > > > >> Please look at the segmentation fault triggered by the following query > since > >> 4d969b2f8: > >> CREATE TABLE t1(a text COMPRESSION pglz); > >> CREATE TABLE t2(a text); > >> CREATE TABLE t3() INHERITS(t1, t2); > >> NOTICE: merging multiple inherited definitions of column "a" > >> server closed the connection unexpectedly > >> This probably means the server terminated abnormally > >> before or while processing the request. > >> > >> Core was generated by `postgres: law regression [local] CREATE TABLE > '. > >> Program terminated with signal SIGSEGV, Segmentation fault. > >> > >> (gdb) bt > >> #0 __strcmp_avx2 () at ../sysdeps/x86_64/multiarch/strcmp-avx2.S:116 > >> #1 0x5606fbcc9d52 in MergeAttributes (columns=0x0, > supers=supers@entry=0x5606fe293d30, relpersistence=112 'p', > >> is_partition=false, supconstr=supconstr@entry=0x7fff4046d410, > supnotnulls=supnotnulls@entry=0x7fff4046d418) > >> at tablecmds.c:2811 > >> #2 0x5606fbccd764 in DefineRelation (stmt=stmt@entry=0x5606fe26a130, > relkind=relkind@entry=114 'r', ownerId=10, > >> ownerId@entry=0, typaddress=typaddress@entry=0x0, > >> queryString=queryString@entry=0x5606fe2695c0 "CREATE TABLE t3() > INHERITS(t1, t2);") at tablecmds.c:885 > > This bug existed even before the refactoring.Happens because strcmp() > > is called on NULL input (t2's compression is NULL). I already have a > > fix for this and will be posting it in [1]. > > > > [1] > https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7%40eisentraut.org > > > > Now that that fix is closed with RwF [1], shouldn't this crash issue be > added to Open Items for v17? > (I couldn't reproduce the crash on 4d969b2f8~1 nor on REL_16_STABLE.) > > https://commitfest.postgresql.org/47/4813/ Yes please. Probably this issue surfaced again after we reverted compression and storage fix? Please If that's the case, please add it to the open items. -- Best Wishes, Ashutosh Bapat
Re: tablecmds.c/MergeAttributes() cleanup
Hello, 30.01.2024 09:22, Ashutosh Bapat wrote: Please look at the segmentation fault triggered by the following query since 4d969b2f8: CREATE TABLE t1(a text COMPRESSION pglz); CREATE TABLE t2(a text); CREATE TABLE t3() INHERITS(t1, t2); NOTICE: merging multiple inherited definitions of column "a" server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. Core was generated by `postgres: law regression [local] CREATE TABLE '. Program terminated with signal SIGSEGV, Segmentation fault. (gdb) bt #0 __strcmp_avx2 () at ../sysdeps/x86_64/multiarch/strcmp-avx2.S:116 #1 0x5606fbcc9d52 in MergeAttributes (columns=0x0, supers=supers@entry=0x5606fe293d30, relpersistence=112 'p', is_partition=false, supconstr=supconstr@entry=0x7fff4046d410, supnotnulls=supnotnulls@entry=0x7fff4046d418) at tablecmds.c:2811 #2 0x5606fbccd764 in DefineRelation (stmt=stmt@entry=0x5606fe26a130, relkind=relkind@entry=114 'r', ownerId=10, ownerId@entry=0, typaddress=typaddress@entry=0x0, queryString=queryString@entry=0x5606fe2695c0 "CREATE TABLE t3() INHERITS(t1, t2);") at tablecmds.c:885 This bug existed even before the refactoring.Happens because strcmp() is called on NULL input (t2's compression is NULL). I already have a fix for this and will be posting it in [1]. [1] https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7%40eisentraut.org Now that that fix is closed with RwF [1], shouldn't this crash issue be added to Open Items for v17? (I couldn't reproduce the crash on 4d969b2f8~1 nor on REL_16_STABLE.) https://commitfest.postgresql.org/47/4813/ Best regards, Alexander
Re: tablecmds.c/MergeAttributes() cleanup
Hi Alexander, On Sun, Jan 28, 2024 at 1:30 PM Alexander Lakhin wrote: > > Hello Peter, > > 26.01.2024 16:42, Peter Eisentraut wrote: > > > > I have committed all this. These are great improvements. > > > > Please look at the segmentation fault triggered by the following query since > 4d969b2f8: > CREATE TABLE t1(a text COMPRESSION pglz); > CREATE TABLE t2(a text); > CREATE TABLE t3() INHERITS(t1, t2); > NOTICE: merging multiple inherited definitions of column "a" > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > > Core was generated by `postgres: law regression [local] CREATE TABLE >'. > Program terminated with signal SIGSEGV, Segmentation fault. > > (gdb) bt > #0 __strcmp_avx2 () at ../sysdeps/x86_64/multiarch/strcmp-avx2.S:116 > #1 0x5606fbcc9d52 in MergeAttributes (columns=0x0, > supers=supers@entry=0x5606fe293d30, relpersistence=112 'p', > is_partition=false, supconstr=supconstr@entry=0x7fff4046d410, > supnotnulls=supnotnulls@entry=0x7fff4046d418) > at tablecmds.c:2811 > #2 0x5606fbccd764 in DefineRelation (stmt=stmt@entry=0x5606fe26a130, > relkind=relkind@entry=114 'r', ownerId=10, > ownerId@entry=0, typaddress=typaddress@entry=0x0, > queryString=queryString@entry=0x5606fe2695c0 "CREATE TABLE t3() > INHERITS(t1, t2);") at tablecmds.c:885 This bug existed even before the refactoring.Happens because strcmp() is called on NULL input (t2's compression is NULL). I already have a fix for this and will be posting it in [1]. [1] https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7%40eisentraut.org -- Best Wishes, Ashutosh Bapat
Re: tablecmds.c/MergeAttributes() cleanup
Hello Peter, 26.01.2024 16:42, Peter Eisentraut wrote: I have committed all this. These are great improvements. Please look at the segmentation fault triggered by the following query since 4d969b2f8: CREATE TABLE t1(a text COMPRESSION pglz); CREATE TABLE t2(a text); CREATE TABLE t3() INHERITS(t1, t2); NOTICE: merging multiple inherited definitions of column "a" server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. Core was generated by `postgres: law regression [local] CREATE TABLE '. Program terminated with signal SIGSEGV, Segmentation fault. (gdb) bt #0 __strcmp_avx2 () at ../sysdeps/x86_64/multiarch/strcmp-avx2.S:116 #1 0x5606fbcc9d52 in MergeAttributes (columns=0x0, supers=supers@entry=0x5606fe293d30, relpersistence=112 'p', is_partition=false, supconstr=supconstr@entry=0x7fff4046d410, supnotnulls=supnotnulls@entry=0x7fff4046d418) at tablecmds.c:2811 #2 0x5606fbccd764 in DefineRelation (stmt=stmt@entry=0x5606fe26a130, relkind=relkind@entry=114 'r', ownerId=10, ownerId@entry=0, typaddress=typaddress@entry=0x0, queryString=queryString@entry=0x5606fe2695c0 "CREATE TABLE t3() INHERITS(t1, t2);") at tablecmds.c:885 ... Best regards, Alexander
Re: tablecmds.c/MergeAttributes() cleanup
On 24.01.24 07:27, Ashutosh Bapat wrote: While working on identity support and now while looking at the compression problem you referred to, I found MergeAttribute() to be hard to read. It's hard to follow high level logic in that function since the function is not modular. I took a stab at modularising a part of it. Attached is the resulting patch series. 0001 is your patch as is 0002 is pgindent fix and also fixing what I think is a typo/thinko from 0001. If you are fine with the changes, 0002 should be merged into 0003. 0003 separates the part of code merging a child attribute to the corresponding inherited attribute into its own function. 0004 does the same for code merging inherited attributes incrementally. I have kept 0003 and 0004 separate in case we pick one and not the other. But they can be committed as a single commit. I have committed all this. These are great improvements. (One little change I made to your 0003 and 0004 patches is that I kept the check whether the new column matches an existing one by name in MergeAttributes(). I found that pushing that down made the logic in MergeAttributes() too hard to follow. But it's pretty much the same.)
Re: tablecmds.c/MergeAttributes() cleanup
Hi Peter, On Mon, Jan 22, 2024 at 6:13 PM Peter Eisentraut wrote: > > On 06.12.23 09:23, Peter Eisentraut wrote: > > The (now) second patch is also still of interest to me, but I have since > > noticed that I think [0] should be fixed first, but to make that fix > > simpler, I would like the first patch from here. > > > > [0]: > > https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7%40eisentraut.org > > The remaining patch in this series needed a rebase and adjustment. > > The above precondition still applies. While working on identity support and now while looking at the compression problem you referred to, I found MergeAttribute() to be hard to read. It's hard to follow high level logic in that function since the function is not modular. I took a stab at modularising a part of it. Attached is the resulting patch series. 0001 is your patch as is 0002 is pgindent fix and also fixing what I think is a typo/thinko from 0001. If you are fine with the changes, 0002 should be merged into 0003. 0003 separates the part of code merging a child attribute to the corresponding inherited attribute into its own function. 0004 does the same for code merging inherited attributes incrementally. I have kept 0003 and 0004 separate in case we pick one and not the other. But they can be committed as a single commit. The two new functions have some common code and some differences. Common code is not surprising since merging attributes whether from child definition or from inheritance parents will have common rules. Differences are expected in cases when child attributes need to be treated differently. But the differences may point us to some yet-unknown bugs; compression being one of those differences. I think the next step should combine these functions into one so that all the logic to merge one attribute is at one place. I haven't attempted it; wanted to propose the idea first. I can see that this moduralization will lead to another and we will be able to reduce MergeAttribute() to a set of function calls reflecting its high level logic and push the detailed implementation into minion functions like this. -- Best Wishes, Ashutosh Bapat From 2553d082b16db2c54f2e1e4a66f1a57ecabf3c1e Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat Date: Wed, 24 Jan 2024 11:15:15 +0530 Subject: [PATCH 2/4] Mark NULL constraint in merged definition instead of new definition This is a typo/thinko in previous commit. Also fix pgindent issue. Ashutosh Bapat --- src/backend/commands/tablecmds.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 04e674bbda..cde524083e 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -2726,8 +2726,8 @@ MergeAttributes(List *columns, const List *supers, char relpersistence, /* * Regular inheritance children are independent enough not to - * inherit identity columns. But partitions are integral part - * of a partitioned table and inherit identity column. + * inherit identity columns. But partitions are integral part of + * a partitioned table and inherit identity column. */ if (is_partition) newdef->identity = attribute->attidentity; @@ -2844,7 +2844,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence, if (bms_is_member(parent_attno, nncols) || bms_is_member(parent_attno - FirstLowInvalidHeapAttributeNumber, pkattrs)) - newdef->is_not_null = true; + prevdef->is_not_null = true; /* * Check for GENERATED conflicts -- 2.25.1 From 092828a7d7274b48bf33c88863a97585bff80ebb Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat Date: Tue, 23 Jan 2024 17:00:43 +0530 Subject: [PATCH 4/4] Separate function to merge next parent attribute Partition inherit from only a single parent. The logic to merge an attribute from the next parent into the corresponding attribute inherited from previous parents in MergeAttribute() is only applicable to regular inheritance children. This code is isolated enough that it can be separate into a function by itself. This separation makes MergeAttributes() more readable making it easy to follow high level logic without getting entangled into details. This separation revealed that the code handling NOT NULL constraints is duplicated in blocks merging the attribute definition incrementally. Deduplicate that code. Author: Ashutosh Bapat --- src/backend/commands/tablecmds.c | 352 --- 1 file changed, 178 insertions(+), 174 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 843cc3bff6..3f0066b435 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -360,6 +360,8 @@ static List *MergeAttributes(List *columns, const List *supers, char relpersiste List **supnotnulls); static List *MergeCheckConstraint(List
Re: tablecmds.c/MergeAttributes() cleanup
On 06.12.23 09:23, Peter Eisentraut wrote: The (now) second patch is also still of interest to me, but I have since noticed that I think [0] should be fixed first, but to make that fix simpler, I would like the first patch from here. [0]: https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7%40eisentraut.org The remaining patch in this series needed a rebase and adjustment. The above precondition still applies. From c4c4462fa58c73fc383c4935c8d2753b7a0b4c9d Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 22 Jan 2024 13:23:43 +0100 Subject: [PATCH v5] MergeAttributes: convert pg_attribute back to ColumnDef before comparing MergeAttributes() has a loop to merge multiple inheritance parents into a column column definition. The merged-so-far definition is stored in a ColumnDef node. If we have to merge columns from multiple inheritance parents (if the name matches), then we have to check whether various column properties (type, collation, etc.) match. The current code extracts the pg_attribute value of the currently-considered inheritance parent and compares it against the merged-so-far ColumnDef value. If the currently considered column doesn't match any previously inherited column, we make a new ColumnDef node from the pg_attribute information and add it to the column list. This patch rearranges this so that we create the ColumnDef node first in either case. Then the code that checks the column properties for compatibility compares ColumnDef against ColumnDef (instead of ColumnDef against pg_attribute). This makes the code more symmetric and easier to follow. Also, later in MergeAttributes(), there is a similar but separate loop that merges the new local column definition with the combination of the inheritance parents established in the first loop. That comparison is already ColumnDef-vs-ColumnDef. With this change, both of these can use similar-looking logic. (A future project might be to extract these two sets of code into a common routine that encodes all the knowledge of whether two column definitions are compatible. But this isn't currently straightforward because we want to give different error messages in the two cases.) Furthermore, by avoiding the use of Form_pg_attribute here, we make it easier to make changes in the pg_attribute layout without having to worry about the local needs of tablecmds.c. Because MergeAttributes() is hugely long, it's sometimes hard to know where in the function you are currently looking. To help with that, I also renamed some variables to make it clearer where you are currently looking. The first look is "prev" vs. "new", the second loop is "inh" vs. "new". Discussion: https://www.postgresql.org/message-id/flat/52a125e4-ff9a-95f5-9f61-b87cf447e...@eisentraut.org --- src/backend/commands/tablecmds.c | 198 --- 1 file changed, 102 insertions(+), 96 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 2a56a4357c9..04e674bbdae 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -2704,7 +2704,8 @@ MergeAttributes(List *columns, const List *supers, char relpersistence, parent_attno - 1); char *attributeName = NameStr(attribute->attname); int exist_attno; - ColumnDef *def; + ColumnDef *newdef; + ColumnDef *savedef; /* * Ignore dropped columns in the parent. @@ -2713,14 +2714,38 @@ MergeAttributes(List *columns, const List *supers, char relpersistence, continue; /* leave newattmap->attnums entry as zero */ /* -* Does it conflict with some previously inherited column? +* Create new column definition +*/ + newdef = makeColumnDef(attributeName, attribute->atttypid, + attribute->atttypmod, attribute->attcollation); + newdef->storage = attribute->attstorage; + newdef->generated = attribute->attgenerated; + if (CompressionMethodIsValid(attribute->attcompression)) + newdef->compression = + pstrdup(GetCompressionMethodName(attribute->attcompression)); + + /* +* Regular inheritance children are independent enough not to +* inherit identity columns. But partitions are integral part +* of a partitioned table and inherit identity column. +
Re: tablecmds.c/MergeAttributes() cleanup
On Fri, Jan 12, 2024 at 10:09 AM Robert Haas wrote: > Open-coding stuff like this can easily work out to a loss, and I > personally think we're overly dependent on List. It's not a > particularly good abstraction, IMHO, and if we do a lot of work to > start using it everywhere because a list is really an array, then what > happens when somebody decides that a list really ought to be a > skip-list, or a hash table, or some other crazy thing? This paragraph was a bit garbled. I meant to say that open-coding can be better than relying on a canned abstraction, but it came out the other way around. -- Robert Haas EDB: http://www.enterprisedb.com
Re: tablecmds.c/MergeAttributes() cleanup
Robert Haas writes: > On Fri, Jan 12, 2024 at 5:32 AM Alvaro Herrera > wrote: >> In addition, it also occurs to me now that maybe it would make sense to >> change the TupleDesc implementation to use a List of Form_pg_attribute >> instead of an array, and do away with ->natts. This would let us change >> all "for ( ... natts ...)" loops into foreach_ptr() loops ... there are >> only five hundred of them or so --rolls eyes--. > Open-coding stuff like this can easily work out to a loss, and I > personally think we're overly dependent on List. It's not a > particularly good abstraction, IMHO, and if we do a lot of work to > start using it everywhere because a list is really an array, then what > happens when somebody decides that a list really ought to be a > skip-list, or a hash table, or some other crazy thing? Without getting into opinions on whether List is a good abstraction, I'm -1 on this idea. It would be a large amount of code churn, with attendant back-patching pain, and I just don't see that there is much to be gained. regards, tom lane
Re: tablecmds.c/MergeAttributes() cleanup
On Fri, Jan 12, 2024 at 5:32 AM Alvaro Herrera wrote: > On 2024-Jan-11, Alvaro Herrera wrote: > > If you look closely at InsertPgAttributeTuples and accompanying code, it > > all looks a bit archaic. They seem to be treating TupleDesc as a > > glorified array of Form_pg_attribute elements in a convenient packaging. > > It's probably cleaner to change these APIs so that they deal with a > > Form_pg_attribute array, and not TupleDesc anymore. But we can hack on > > that some other day. > > In addition, it also occurs to me now that maybe it would make sense to > change the TupleDesc implementation to use a List of Form_pg_attribute > instead of an array, and do away with ->natts. This would let us change > all "for ( ... natts ...)" loops into foreach_ptr() loops ... there are > only five hundred of them or so --rolls eyes--. Open-coding stuff like this can easily work out to a loss, and I personally think we're overly dependent on List. It's not a particularly good abstraction, IMHO, and if we do a lot of work to start using it everywhere because a list is really an array, then what happens when somebody decides that a list really ought to be a skip-list, or a hash table, or some other crazy thing? -- Robert Haas EDB: http://www.enterprisedb.com
Re: tablecmds.c/MergeAttributes() cleanup
On 2024-Jan-11, Alvaro Herrera wrote: > If you look closely at InsertPgAttributeTuples and accompanying code, it > all looks a bit archaic. They seem to be treating TupleDesc as a > glorified array of Form_pg_attribute elements in a convenient packaging. > It's probably cleaner to change these APIs so that they deal with a > Form_pg_attribute array, and not TupleDesc anymore. But we can hack on > that some other day. In addition, it also occurs to me now that maybe it would make sense to change the TupleDesc implementation to use a List of Form_pg_attribute instead of an array, and do away with ->natts. This would let us change all "for ( ... natts ...)" loops into foreach_ptr() loops ... there are only five hundred of them or so --rolls eyes--. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "El sudor es la mejor cura para un pensamiento enfermo" (Bardia)
Re: tablecmds.c/MergeAttributes() cleanup
On 2023-Dec-06, Peter Eisentraut wrote: > One of your (Álvaro's) comments about this earlier was > > > Hmm, crazy. I'm not sure I like this, because it seems much too clever. > > The number of lines that are deleted is alluring, though. > > > > Maybe it'd be better to create a separate routine that takes a single > > ColumnDef and returns the Form_pg_attribute element for it; then use > > that in both BuildDescForRelation and ATExecAddColumn. > > which was also my thought at the beginning. However, this wouldn't quite > work that way, for several reasons. For instance, BuildDescForRelation() > also needs to keep track of the has_not_null property across all fields, and > so if you split that function up, you would have to somehow make that an > output argument and have the caller keep track of it. Also, the output of > BuildDescForRelation() in ATExecAddColumn() is passed into > InsertPgAttributeTuples(), which requires a tuple descriptor anyway, so > splitting this up into a per-attribute function would then require > ATExecAddColumn() to re-assemble a tuple descriptor anyway, so this wouldn't > save anything. Hmm. Well, if this state of affairs is useful to you, then I withdraw my objection, because with this patch we're not really adding any new weirdness, just moving around already-existing weirdness. So let's press ahead with 0001. (I didn't look at 0002 this time, since apparently you'd like to process the other patch first and then come back here.) If you look closely at InsertPgAttributeTuples and accompanying code, it all looks a bit archaic. They seem to be treating TupleDesc as a glorified array of Form_pg_attribute elements in a convenient packaging. It's probably cleaner to change these APIs so that they deal with a Form_pg_attribute array, and not TupleDesc anymore. But we can hack on that some other day. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Pensar que el espectro que vemos es ilusorio no lo despoja de espanto, sólo le suma el nuevo terror de la locura" (Perelandra, C.S. Lewis)
Re: tablecmds.c/MergeAttributes() cleanup
On 05.10.23 17:49, Peter Eisentraut wrote: On 19.09.23 15:11, Peter Eisentraut wrote: Here is an updated version of this patch set. I resolved some conflicts and addressed this comment of yours. I also dropped the one patch with some catalog header edits that people didn't seem to particularly like. The patches that are now 0001 through 0004 were previously reviewed and just held for the not-null constraint patches, I think, so I'll commit them in a few days if there are no objections. Patches 0005 through 0007 are also ready in my opinion, but they haven't really been reviewed, so this would be something for reviewers to focus on. (0005 and 0007 are trivial, but they go to together with 0006.) The remaining 0008 and 0009 were still under discussion and contemplation. I have committed through 0007, and I'll now close this patch set as "Committed", and I will (probably) bring back the rest (especially 0008) as part of a different patch set soon. After playing with this for, well, 2 months, and considering various other approaches, I would like to bring back the remaining two patches in unchanged form. Especially the (now) first patch "Refactor ATExecAddColumn() to use BuildDescForRelation()" would be very helpful for facilitating further refactoring in this area, because it avoids having two essentially duplicate pieces of code responsible for converting a ColumnDef node into internal form. One of your (Álvaro's) comments about this earlier was > Hmm, crazy. I'm not sure I like this, because it seems much too clever. > The number of lines that are deleted is alluring, though. > > Maybe it'd be better to create a separate routine that takes a single > ColumnDef and returns the Form_pg_attribute element for it; then use > that in both BuildDescForRelation and ATExecAddColumn. which was also my thought at the beginning. However, this wouldn't quite work that way, for several reasons. For instance, BuildDescForRelation() also needs to keep track of the has_not_null property across all fields, and so if you split that function up, you would have to somehow make that an output argument and have the caller keep track of it. Also, the output of BuildDescForRelation() in ATExecAddColumn() is passed into InsertPgAttributeTuples(), which requires a tuple descriptor anyway, so splitting this up into a per-attribute function would then require ATExecAddColumn() to re-assemble a tuple descriptor anyway, so this wouldn't save anything. Also note that ATExecAddColumn() could in theory be enhanced to add more than one column in one go, so having this code structure in place isn't inconsistent with that. The main hackish thing, I suppose, is that we have to fix up the attribute number after returning from BuildDescForRelation(). I suppose we could address that by passing in a starting attribute number (or alternatively maximum existing attribute number) into BuildDescForRelation(). I think that would be okay; it would probably be about a wash in terms of code added versus saved. The (now) second patch is also still of interest to me, but I have since noticed that I think [0] should be fixed first, but to make that fix simpler, I would like the first patch from here. [0]: https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7%40eisentraut.org From 42615f4c523920c7ae916ba0b1819cc6a5e622b1 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 5 Oct 2023 16:17:16 +0200 Subject: [PATCH v4 1/2] Refactor ATExecAddColumn() to use BuildDescForRelation() BuildDescForRelation() has all the knowledge for converting a ColumnDef into pg_attribute/tuple descriptor. ATExecAddColumn() can make use of that, instead of duplicating all that logic. We just pass a one-element list of ColumnDef and we get back exactly the data structure we need. Note that we don't even need to touch BuildDescForRelation() to make this work. Discussion: https://www.postgresql.org/message-id/flat/52a125e4-ff9a-95f5-9f61-b87cf447e...@eisentraut.org --- src/backend/commands/tablecmds.c | 89 1 file changed, 22 insertions(+), 67 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 6b0a20010e..875cfeaa5a 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -6965,22 +6965,15 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, Relationpgclass, attrdesc; HeapTuple reltup; - FormData_pg_attribute attribute; + Form_pg_attribute attribute; int newattnum; charrelkind; - HeapTuple typeTuple; - Oid typeOid; - int32 typmod; - Oid collOid; - Form_pg_type tform; Expr *defval; List *children; ListCell
Re: tablecmds.c/MergeAttributes() cleanup
On 19.09.23 15:11, Peter Eisentraut wrote: Here is an updated version of this patch set. I resolved some conflicts and addressed this comment of yours. I also dropped the one patch with some catalog header edits that people didn't seem to particularly like. The patches that are now 0001 through 0004 were previously reviewed and just held for the not-null constraint patches, I think, so I'll commit them in a few days if there are no objections. Patches 0005 through 0007 are also ready in my opinion, but they haven't really been reviewed, so this would be something for reviewers to focus on. (0005 and 0007 are trivial, but they go to together with 0006.) The remaining 0008 and 0009 were still under discussion and contemplation. I have committed through 0007, and I'll now close this patch set as "Committed", and I will (probably) bring back the rest (especially 0008) as part of a different patch set soon.
Re: tablecmds.c/MergeAttributes() cleanup
On 29.08.23 13:20, Alvaro Herrera wrote:
On 2023-Aug-29, Peter Eisentraut wrote:
@@ -3278,13 +3261,16 @@ MergeAttributes(List *schema, List *supers, char
relpersistence,
*
* constraints is a list of CookedConstraint structs for previous constraints.
*
- * Returns true if merged (constraint is a duplicate), or false if it's
- * got a so-far-unique name, or throws error if conflict.
+ * If the constraint is a duplicate, then the existing constraint's
+ * inheritance count is updated. If the constraint doesn't match or conflict
+ * with an existing one, a new constraint is appended to the list. If there
+ * is a conflict (same name but different expression), throw an error.
This wording confused me:
"If the constraint doesn't match or conflict with an existing one, a new
constraint is appended to the list."
I first read it as "doesn't match or conflicts with ..." (i.e., the
negation only applied to the first verb, not both) which would have been
surprising (== broken) behavior.
I think it's clearer if you say "doesn't match nor conflict", but I'm
not sure if this is grammatically correct.
Here is an updated version of this patch set. I resolved some conflicts
and addressed this comment of yours. I also dropped the one patch with
some catalog header edits that people didn't seem to particularly like.
The patches that are now 0001 through 0004 were previously reviewed and
just held for the not-null constraint patches, I think, so I'll commit
them in a few days if there are no objections.
Patches 0005 through 0007 are also ready in my opinion, but they haven't
really been reviewed, so this would be something for reviewers to focus
on. (0005 and 0007 are trivial, but they go to together with 0006.)
The remaining 0008 and 0009 were still under discussion and contemplation.From 28e4dbba35fc3162c13f5896551921896cf30d1c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 12 Jul 2023 16:12:34 +0200
Subject: [PATCH v3 1/9] Clean up MergeAttributesIntoExisting()
Make variable naming clearer and more consistent. Move some variables
to smaller scope. Remove some unnecessary intermediate variables.
Try to save some vertical space.
Apply analogous changes to nearby MergeConstraintsIntoExisting() and
RemoveInheritance() for consistency.
---
src/backend/commands/tablecmds.c | 123 ---
1 file changed, 48 insertions(+), 75 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a2c671b66..ff001f5ceb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15695,53 +15695,39 @@ static void
MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
{
Relationattrrel;
- AttrNumber parent_attno;
- int parent_natts;
- TupleDesc tupleDesc;
- HeapTuple tuple;
- boolchild_is_partition = false;
+ TupleDesc parent_desc;
attrrel = table_open(AttributeRelationId, RowExclusiveLock);
+ parent_desc = RelationGetDescr(parent_rel);
- tupleDesc = RelationGetDescr(parent_rel);
- parent_natts = tupleDesc->natts;
-
- /* If parent_rel is a partitioned table, child_rel must be a partition
*/
- if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- child_is_partition = true;
-
- for (parent_attno = 1; parent_attno <= parent_natts; parent_attno++)
+ for (AttrNumber parent_attno = 1; parent_attno <= parent_desc->natts;
parent_attno++)
{
- Form_pg_attribute attribute = TupleDescAttr(tupleDesc,
-
parent_attno - 1);
- char *attributeName = NameStr(attribute->attname);
+ Form_pg_attribute parent_att = TupleDescAttr(parent_desc,
parent_attno - 1);
+ char *parent_attname = NameStr(parent_att->attname);
+ HeapTuple tuple;
/* Ignore dropped columns in the parent. */
- if (attribute->attisdropped)
+ if (parent_att->attisdropped)
continue;
/* Find same column in child (matching on column name). */
- tuple = SearchSysCacheCopyAttName(RelationGetRelid(child_rel),
-
attributeName);
+ tuple = SearchSysCacheCopyAttName(RelationGetRelid(child_rel),
parent_attname);
if (HeapTupleIsValid(tuple))
{
- /* Check they are same type, typmod, and collation */
- Form_pg_attribute childatt = (Form_pg_attribute)
GETSTRUCT(tuple);
+ Form_pg_attribute child_att = (Form_pg_attribute)
GETSTRUCT(tuple);
- if
Re: tablecmds.c/MergeAttributes() cleanup
On 2023-Aug-29, Nathan Bossart wrote:
> On Tue, Aug 29, 2023 at 08:44:02PM +0200, Alvaro Herrera wrote:
> > Makes sense. However, maybe we should replace those ugly defines and
> > their hardcoded values in DefineSequence with a proper array with their
> > names and datatypes.
>
> That might be an improvement, but IIUC you'd still need to enumerate all of
> the columns or data types to make sure you use the right get-Datum
> function. It doesn't help that last_value uses Int64GetDatumFast and
> log_cnt uses Int64GetDatum. I could be missing something, though.
Well, for sure I meant to enumerate everything that was needed,
including the initializer for the value. Like in the attached patch.
However, now that I've actually written it, I don't find it so pretty
anymore, but maybe that's just because I don't know how to write the
array assignment as a single statement instead of a separate statement
for each column.
But this should silence the warnings, anyway.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
commit 50ac445d1e1ad84282d59c09a3937c221d94b968
Author: Alvaro Herrera [Álvaro Herrera ]
AuthorDate: Wed Aug 30 16:09:52 2023 +0200
CommitDate: Wed Aug 30 16:41:06 2023 +0200
seq-warnings
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 0b0003fcc8..4798c9cd20 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -129,11 +129,16 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
Relation rel;
HeapTuple tuple;
TupleDesc tupDesc;
- Datum value[SEQ_COL_LASTCOL];
- bool null[SEQ_COL_LASTCOL];
Datum pgs_values[Natts_pg_sequence];
bool pgs_nulls[Natts_pg_sequence];
- int i;
+ struct SequenceColumn
+ {
+ const char *name;
+ Oid type;
+ Datum init_value;
+ } sequence_cols[3];
+ Datum value[3];
+ bool null[3];
/*
* If if_not_exists was given and a relation with the same name already
@@ -166,32 +171,39 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
, ,
_seq_rewrite, _by);
+ sequence_cols[0] = (struct SequenceColumn)
+ {
+ .name = "last_value",
+ .type = INT8OID,
+ .init_value = Int64GetDatumFast(seqdataform.last_value)
+ };
+ sequence_cols[1] = (struct SequenceColumn)
+ {
+ .name = "log_cnt",
+ .type = INT8OID,
+ .init_value = Int64GetDatum((int64) 0)
+ };
+ sequence_cols[2] = (struct SequenceColumn)
+ {
+ .name = "is_called",
+ .type = BOOLOID,
+ .init_value = BoolGetDatum(false)
+ };
+
/*
* Create relation (and fill value[] and null[] for the tuple)
*/
stmt->tableElts = NIL;
- for (i = SEQ_COL_FIRSTCOL; i <= SEQ_COL_LASTCOL; i++)
+ for (int i = 0; i < lengthof(sequence_cols); i++)
{
ColumnDef *coldef;
- switch (i)
- {
- case SEQ_COL_LASTVAL:
-coldef = makeColumnDef("last_value", INT8OID, -1, InvalidOid);
-value[i - 1] = Int64GetDatumFast(seqdataform.last_value);
-break;
- case SEQ_COL_LOG:
-coldef = makeColumnDef("log_cnt", INT8OID, -1, InvalidOid);
-value[i - 1] = Int64GetDatum((int64) 0);
-break;
- case SEQ_COL_CALLED:
-coldef = makeColumnDef("is_called", BOOLOID, -1, InvalidOid);
-value[i - 1] = BoolGetDatum(false);
-break;
- }
-
+ coldef = makeColumnDef(sequence_cols[i].name, sequence_cols[i].type, -1,
+ InvalidOid);
coldef->is_not_null = true;
- null[i - 1] = false;
+
+ value[i] = sequence_cols[i].init_value;
+ null[i] = false;
stmt->tableElts = lappend(stmt->tableElts, coldef);
}
diff --git a/src/include/commands/sequence.h b/src/include/commands/sequence.h
index 7db7b3da7b..784280b26d 100644
--- a/src/include/commands/sequence.h
+++ b/src/include/commands/sequence.h
@@ -31,17 +31,6 @@ typedef struct FormData_pg_sequence_data
typedef FormData_pg_sequence_data *Form_pg_sequence_data;
-/*
- * Columns of a sequence relation
- */
-
-#define SEQ_COL_LASTVAL 1
-#define SEQ_COL_LOG2
-#define SEQ_COL_CALLED 3
-
-#define SEQ_COL_FIRSTCOL SEQ_COL_LASTVAL
-#define SEQ_COL_LASTCOL SEQ_COL_CALLED
-
/* XLOG stuff */
#define XLOG_SEQ_LOG 0x00
Re: tablecmds.c/MergeAttributes() cleanup
On 29.08.23 19:45, Nathan Bossart wrote: On Tue, Aug 29, 2023 at 10:43:39AM +0200, Peter Eisentraut wrote: I have committed a few more patches from this series that were already agreed upon. The remaining ones are rebased and reordered a bit, attached. My compiler is complaining about 1fa9241b: ../postgresql/src/backend/commands/sequence.c: In function ‘DefineSequence’: ../postgresql/src/backend/commands/sequence.c:196:21: error: ‘coldef’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 196 | stmt->tableElts = lappend(stmt->tableElts, coldef); | ^~~~ This went away when I added a default case that ERROR'd or initialized coldef to NULL. fixed
Re: tablecmds.c/MergeAttributes() cleanup
On Tue, Aug 29, 2023 at 08:44:02PM +0200, Alvaro Herrera wrote: > On 2023-Aug-29, Nathan Bossart wrote: >> My compiler is complaining about 1fa9241b: >> >> ../postgresql/src/backend/commands/sequence.c: In function ‘DefineSequence’: >> ../postgresql/src/backend/commands/sequence.c:196:21: error: ‘coldef’ may be >> used uninitialized in this function [-Werror=maybe-uninitialized] >> 196 | stmt->tableElts = lappend(stmt->tableElts, coldef); >> | ^~~~ >> >> This went away when I added a default case that ERROR'd or initialized >> coldef to NULL. > > Makes sense. However, maybe we should replace those ugly defines and > their hardcoded values in DefineSequence with a proper array with their > names and datatypes. That might be an improvement, but IIUC you'd still need to enumerate all of the columns or data types to make sure you use the right get-Datum function. It doesn't help that last_value uses Int64GetDatumFast and log_cnt uses Int64GetDatum. I could be missing something, though. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: tablecmds.c/MergeAttributes() cleanup
On 2023-Aug-29, Nathan Bossart wrote: > On Tue, Aug 29, 2023 at 10:43:39AM +0200, Peter Eisentraut wrote: > > I have committed a few more patches from this series that were already > > agreed upon. The remaining ones are rebased and reordered a bit, attached. > > My compiler is complaining about 1fa9241b: > > ../postgresql/src/backend/commands/sequence.c: In function ‘DefineSequence’: > ../postgresql/src/backend/commands/sequence.c:196:21: error: ‘coldef’ may be > used uninitialized in this function [-Werror=maybe-uninitialized] > 196 | stmt->tableElts = lappend(stmt->tableElts, coldef); > | ^~~~ > > This went away when I added a default case that ERROR'd or initialized > coldef to NULL. Makes sense. However, maybe we should replace those ugly defines and their hardcoded values in DefineSequence with a proper array with their names and datatypes. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Los trabajadores menos efectivos son sistematicamente llevados al lugar donde pueden hacer el menor daño posible: gerencia." (El principio Dilbert)
Re: tablecmds.c/MergeAttributes() cleanup
On Tue, Aug 29, 2023 at 10:43:39AM +0200, Peter Eisentraut wrote: > I have committed a few more patches from this series that were already > agreed upon. The remaining ones are rebased and reordered a bit, attached. My compiler is complaining about 1fa9241b: ../postgresql/src/backend/commands/sequence.c: In function ‘DefineSequence’: ../postgresql/src/backend/commands/sequence.c:196:21: error: ‘coldef’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 196 | stmt->tableElts = lappend(stmt->tableElts, coldef); | ^~~~ This went away when I added a default case that ERROR'd or initialized coldef to NULL. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: tablecmds.c/MergeAttributes() cleanup
On 2023-Aug-29, Peter Eisentraut wrote: > From 471fda80c41fae835ecbe63ae8505526a37487a9 Mon Sep 17 00:00:00 2001 > From: Peter Eisentraut > Date: Wed, 12 Jul 2023 16:12:35 +0200 > Subject: [PATCH v2 04/10] Add TupleDescGetDefault() > > This unifies some repetitive code. > > Note: I didn't push the "not found" error message into the new > function, even though all existing callers would be able to make use > of it. Using the existing error handling as-is would probably require > exposing the Relation type via tupdesc.h, which doesn't seem > desirable. Note that all errors here are elog(ERROR), not user-facing, so ISTM it would be OK to change them to report the relation OID rather than the name. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "In fact, the basic problem with Perl 5's subroutines is that they're not crufty enough, so the cruft leaks out into user-defined code instead, by the Conservation of Cruft Principle." (Larry Wall, Apocalypse 6)
Re: tablecmds.c/MergeAttributes() cleanup
On 2023-Aug-29, Peter Eisentraut wrote: Regarding this hunk in 0002, > @@ -3278,13 +3261,16 @@ MergeAttributes(List *schema, List *supers, char > relpersistence, > * > * constraints is a list of CookedConstraint structs for previous > constraints. > * > - * Returns true if merged (constraint is a duplicate), or false if it's > - * got a so-far-unique name, or throws error if conflict. > + * If the constraint is a duplicate, then the existing constraint's > + * inheritance count is updated. If the constraint doesn't match or conflict > + * with an existing one, a new constraint is appended to the list. If there > + * is a conflict (same name but different expression), throw an error. This wording confused me: "If the constraint doesn't match or conflict with an existing one, a new constraint is appended to the list." I first read it as "doesn't match or conflicts with ..." (i.e., the negation only applied to the first verb, not both) which would have been surprising (== broken) behavior. I think it's clearer if you say "doesn't match nor conflict", but I'm not sure if this is grammatically correct. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: tablecmds.c/MergeAttributes() cleanup
On 12.07.23 16:29, Peter Eisentraut wrote:
On 11.07.23 20:17, Alvaro Herrera wrote:
I spent a few minutes doing a test merge of this to my branch with NOT
NULL changes. Here's a quick review.
Subject: [PATCH 01/17] Remove obsolete comment about OID support
Obvious, trivial. +1
Subject: [PATCH 02/17] Remove ancient special case code for adding
oid columns
LGTM; deletes dead code.
Subject: [PATCH 03/17] Remove ancient special case code for dropping oid
columns
Hmm, interesting. Yay for more dead code removal. Didn't verify it.
I have committed these first three. I'll leave it at that for now.
I have committed a few more patches from this series that were already
agreed upon. The remaining ones are rebased and reordered a bit, attached.
There was some doubt about the patch "Refactor ATExecAddColumn() to use
BuildDescForRelation()" (now 0009), whether it's too clever to build a
fake one-item tuple descriptor. I am working on another patch, which I
hope to post this week, that proposes to replace the use of tuple
descriptors there with a List of something. That would then allow maybe
rewriting this in a less-clever way. That patch incidentally also wants
to move BuildDescForRelation from tupdesc.c to tablecmds.c (patch 0007
here).
From 4d01d244305c63a5a602558267d0021910e200b6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 12 Jul 2023 16:12:34 +0200
Subject: [PATCH v2 01/10] Clean up MergeAttributesIntoExisting()
Make variable naming clearer and more consistent. Move some variables
to smaller scope. Remove some unnecessary intermediate variables.
Try to save some vertical space.
Apply analogous changes to nearby MergeConstraintsIntoExisting() and
RemoveInheritance() for consistency.
---
src/backend/commands/tablecmds.c | 123 ---
1 file changed, 48 insertions(+), 75 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d097da3c78..e7846178b3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15691,92 +15691,76 @@ static void
MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
{
Relationattrrel;
- AttrNumber parent_attno;
- int parent_natts;
- TupleDesc tupleDesc;
- HeapTuple tuple;
- boolchild_is_partition = false;
+ TupleDesc parent_desc;
attrrel = table_open(AttributeRelationId, RowExclusiveLock);
+ parent_desc = RelationGetDescr(parent_rel);
- tupleDesc = RelationGetDescr(parent_rel);
- parent_natts = tupleDesc->natts;
-
- /* If parent_rel is a partitioned table, child_rel must be a partition
*/
- if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- child_is_partition = true;
-
- for (parent_attno = 1; parent_attno <= parent_natts; parent_attno++)
+ for (AttrNumber parent_attno = 1; parent_attno <= parent_desc->natts;
parent_attno++)
{
- Form_pg_attribute attribute = TupleDescAttr(tupleDesc,
-
parent_attno - 1);
- char *attributeName = NameStr(attribute->attname);
+ Form_pg_attribute parent_att = TupleDescAttr(parent_desc,
parent_attno - 1);
+ char *parent_attname = NameStr(parent_att->attname);
+ HeapTuple tuple;
/* Ignore dropped columns in the parent. */
- if (attribute->attisdropped)
+ if (parent_att->attisdropped)
continue;
/* Find same column in child (matching on column name). */
- tuple = SearchSysCacheCopyAttName(RelationGetRelid(child_rel),
-
attributeName);
+ tuple = SearchSysCacheCopyAttName(RelationGetRelid(child_rel),
parent_attname);
if (HeapTupleIsValid(tuple))
{
- /* Check they are same type, typmod, and collation */
- Form_pg_attribute childatt = (Form_pg_attribute)
GETSTRUCT(tuple);
+ Form_pg_attribute child_att = (Form_pg_attribute)
GETSTRUCT(tuple);
- if (attribute->atttypid != childatt->atttypid ||
- attribute->atttypmod != childatt->atttypmod)
+ if (parent_att->atttypid != child_att->atttypid ||
+ parent_att->atttypmod != child_att->atttypmod)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("child table \"%s\" has
different type for column \"%s\"",
-
Re: tablecmds.c/MergeAttributes() cleanup
On 11.07.23 20:17, Alvaro Herrera wrote: I spent a few minutes doing a test merge of this to my branch with NOT NULL changes. Here's a quick review. Subject: [PATCH 01/17] Remove obsolete comment about OID support Obvious, trivial. +1 Subject: [PATCH 02/17] Remove ancient special case code for adding oid columns LGTM; deletes dead code. Subject: [PATCH 03/17] Remove ancient special case code for dropping oid columns Hmm, interesting. Yay for more dead code removal. Didn't verify it. I have committed these first three. I'll leave it at that for now. Subject: [PATCH 08/17] Improve some catalog documentation Point out that typstorage and attstorage are never '\0', even for fixed-length types. This is different from attcompression. For this reason, some of the handling of these columns in tablecmds.c etc. is different. (catalogs.sgml already contained this information in an indirect way.) I don't understand why we must point out that they're never '\0'. I mean, if we're doing that, why not say that they can never be \xFF? The valid values are already listed. The other parts of this patch look okay. While working through the storage and compression handling, which look similar, I was momentarily puzzled by this. While attcompression can be 0 to mean, use default, this is not possible/allowed for attstorage, but it took looking around three corners to verify this. It could be more explicit, I thought.
Re: tablecmds.c/MergeAttributes() cleanup
On 2023-Jun-28, Peter Eisentraut wrote: > The MergeAttributes() and related code in and around tablecmds.c is huge and > ancient, with many things bolted on over time, and difficult to deal with. > I took some time to make careful incremental updates and refactorings to > make the code easier to follow, more compact, and more modern in appearance. > I also found several pieces of obsolete code along the way. This resulted > in the attached long patch series. Each patch tries to make a single change > and can be considered incrementally. At the end, the code is shorter, > better factored, and I hope easier to understand. There shouldn't be any > change in behavior. I spent a few minutes doing a test merge of this to my branch with NOT NULL changes. Here's a quick review. > Subject: [PATCH 01/17] Remove obsolete comment about OID support Obvious, trivial. +1 > Subject: [PATCH 02/17] Remove ancient special case code for adding oid columns LGTM; deletes dead code. > Subject: [PATCH 03/17] Remove ancient special case code for dropping oid > columns Hmm, interesting. Yay for more dead code removal. Didn't verify it. > Subject: [PATCH 04/17] Make more use of makeColumnDef() Good idea, +1. Some lines (almost all makeColumnDef callsites) end up too long. This is the first patch that actually conflicts with the NOT NULLs one, and the conflicts are easy to solve, so I won't complain if you want to get it pushed soon. > Subject: [PATCH 05/17] Clean up MergeAttributesIntoExisting() I don't disagree with this in principle, but this one has more conflicts than the previous ones. > Subject: [PATCH 06/17] Clean up MergeCheckConstraint() Looks a reasonable change as far as this patch goes. However, reading it I noticed that CookedConstraint->inhcount is int and is tested for wraparound, but pg_constraint.coninhcount is int16. This is probably bogus already. ColumnDef->inhcount is also int. These should be narrowed to match the catalog definitions. > Subject: [PATCH 07/17] MergeAttributes() and related variable renaming I think this makes sense, but there's a bunch of conflicts to NOT NULLs. I guess we can come back to this one later. > Subject: [PATCH 08/17] Improve some catalog documentation > > Point out that typstorage and attstorage are never '\0', even for > fixed-length types. This is different from attcompression. For this > reason, some of the handling of these columns in tablecmds.c etc. is > different. (catalogs.sgml already contained this information in an > indirect way.) I don't understand why we must point out that they're never '\0'. I mean, if we're doing that, why not say that they can never be \xFF? The valid values are already listed. The other parts of this patch look okay. > Subject: [PATCH 09/17] Remove useless if condition > > This is useless because these fields are not set anywhere before, so > we can assign them unconditionally. This also makes this more > consistent with ATExecAddColumn(). Makes sense. > Subject: [PATCH 10/17] Remove useless if condition > > We can call GetAttributeCompression() with a NULL argument. It > handles that internally already. This change makes all the callers of > GetAttributeCompression() uniform. I agree, +1. > From 2eda6bc9897d0995a5112e2851c51daf0c35656e Mon Sep 17 00:00:00 2001 > From: Peter Eisentraut > Date: Wed, 14 Jun 2023 17:51:31 +0200 > Subject: [PATCH 11/17] Refactor ATExecAddColumn() to use > BuildDescForRelation() > > BuildDescForRelation() has all the knowledge for converting a > ColumnDef into pg_attribute/tuple descriptor. ATExecAddColumn() can > make use of that, instead of duplicating all that logic. We just pass > a one-element list of ColumnDef and we get back exactly the data > structure we need. Note that we don't even need to touch > BuildDescForRelation() to make this work. Hmm, crazy. I'm not sure I like this, because it seems much too clever. The number of lines that are deleted is alluring, though. Maybe it'd be better to create a separate routine that takes a single ColumnDef and returns the Form_pg_attribute element for it; then use that in both BuildDescForRelation and ATExecAddColumn. > Subject: [PATCH 12/17] Push attidentity and attgenerated handling into > BuildDescForRelation() > > Previously, this was handled by the callers separately, but it can be > trivially moved into BuildDescForRelation() so that it is handled in a > central place. Looks reasonable. I think the last few patches are the more innovative (interesting, useful) of the bunch. Let's get the first few out of the way. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: tablecmds.c/MergeAttributes() cleanup
On 2023-Jun-28, Peter Eisentraut wrote: > The MergeAttributes() and related code in and around tablecmds.c is huge and > ancient, with many things bolted on over time, and difficult to deal with. > I took some time to make careful incremental updates and refactorings to > make the code easier to follow, more compact, and more modern in appearance. > I also found several pieces of obsolete code along the way. This resulted > in the attached long patch series. Each patch tries to make a single change > and can be considered incrementally. At the end, the code is shorter, > better factored, and I hope easier to understand. There shouldn't be any > change in behavior. I request to leave this alone for now. I have enough things to juggle with in the NOTNULLs patch; this patchset looks like it will cause me messy merge conflicts. 0004 for instance looks problematic, as does 0007 and 0016. FWIW for the most part that patch is working and I intend to re-submit shortly, but the relevant pg_upgrade code is really brittle, so it's taken me much more than I expected to get it in good shape for all cases. Thanks -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
tablecmds.c/MergeAttributes() cleanup
The MergeAttributes() and related code in and around tablecmds.c is huge
and ancient, with many things bolted on over time, and difficult to deal
with. I took some time to make careful incremental updates and
refactorings to make the code easier to follow, more compact, and more
modern in appearance. I also found several pieces of obsolete code
along the way. This resulted in the attached long patch series. Each
patch tries to make a single change and can be considered incrementally.
At the end, the code is shorter, better factored, and I hope easier to
understand. There shouldn't be any change in behavior.From 60a671aeb03293bdec65fd86f2a393c3aced6eb9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 27 Jun 2023 17:10:25 +0200
Subject: [PATCH 01/17] Remove obsolete comment about OID support
---
src/backend/catalog/heap.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 2a0d82aedd..9196dcd39f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -843,9 +843,9 @@ AddNewAttributeTuples(Oid new_rel_oid,
}
/*
-* Next we add the system attributes. Skip OID if rel has no OIDs. Skip
-* all for a view or type relation. We don't bother with making
datatype
-* dependencies here, since presumably all these types are pinned.
+* Next we add the system attributes. Skip all for a view or type
+* relation. We don't bother with making datatype dependencies here,
+* since presumably all these types are pinned.
*/
if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE)
{
base-commit: b381d9637030c163c3b1f8a9d3de51dfc1b4ee58
--
2.41.0
From e12a69675919fb026c008e37649518f3d52e2a90 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 22 Jun 2023 12:05:06 +0200
Subject: [PATCH 02/17] Remove ancient special case code for adding oid columns
The special handling of negative attribute numbers in
ATExecAddColumn() was introduced to support SET WITH OIDS (commit
6d1e361852). But that feature doesn't exist anymore, so we can revert
to the previous, simpler version. In passing, also remove an obsolete
comment about OID support.
---
src/backend/commands/tablecmds.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d985278ac6..a5493705aa 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2449,8 +2449,7 @@ MergeAttributes(List *schema, List *supers, char
relpersistence,
/*
* Scan the parents left-to-right, and merge their attributes to form a
-* list of inherited attributes (inhSchema). Also check to see if we
need
-* to inherit an OID column.
+* list of inherited attributes (inhSchema).
*/
child_attno = 0;
foreach(entry, supers)
@@ -6944,7 +6943,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab,
Relation rel,
attribute.attrelid = myrelid;
namestrcpy(&(attribute.attname), colDef->colname);
attribute.atttypid = typeOid;
- attribute.attstattarget = (newattnum > 0) ? -1 : 0;
+ attribute.attstattarget = -1;
attribute.attlen = tform->typlen;
attribute.attnum = newattnum;
if (list_length(colDef->typeName->arrayBounds) > PG_INT16_MAX)
@@ -7067,7 +7066,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab,
Relation rel,
* is certainly not going to touch them. System attributes don't have
* interesting defaults, either.
*/
- if (RELKIND_HAS_STORAGE(relkind) && attribute.attnum > 0)
+ if (RELKIND_HAS_STORAGE(relkind))
{
/*
* For an identity column, we can't use build_column_default(),
--
2.41.0
From 58af867b3e3990e787a33c5d5023753e623dffe0 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 27 Jun 2023 14:43:55 +0200
Subject: [PATCH 03/17] Remove ancient special case code for dropping oid
columns
The special handling of negative attribute numbers in
RemoveAttributeById() was introduced to support SET WITHOUT OIDS
(commit 24614a9880). But that feature doesn't exist anymore, so we
can revert to the previous, simpler version.
---
src/backend/catalog/heap.c | 99 +-
1 file changed, 43 insertions(+), 56 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 9196dcd39f..4c30c7d461 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1666,68 +1666,56 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
attnum, relid);
attStruct = (Form_pg_attribute) GETSTRUCT(tuple);
- if (attnum < 0)
- {
- /* System attribute (probably OID) ... just delete the row */
-
-
