public inbox for [email protected]help / color / mirror / Atom feed
TupleDescAttr bounds checks 10+ messages / 4 participants [nested] [flat]
* TupleDescAttr bounds checks @ 2026-03-20 15:58 Robert Haas <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Robert Haas @ 2026-03-20 15:58 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi, Scrutiny of a recent test_plan_advice failure in the buildfarm revealed a bug that had nothing to do with test_plan_advice or pg_plan_advice; rather, it was a bug introduced by the virtual generated columns feature, and specifically of that feature indexing off of the beginning of a TupleDesc when whole-row attributes are present. The first patch attached to this email fixes this issue, and should be committed and back-patched to v18. I plan to do that soon unless there are objections. But that got me wondering why we don't have an assertion in TupleDescAttr to catch this sort of thing, and it seems like that is indeed something we can do, so patch #2 adds that and then cleans up the resulting damage. By "damage" I mean correcting places where the new Assert() either actually fails or could theoretically fail, because we use TupleDescAttr() on a value that we don't know to be within range. None of these seem to be actual bugs, because as the commit message says, all TupleDescAttr() does is compute a pointer, and we don't actually dereference that pointer in any of these code paths until after we know that it's OK to do so. Nonetheless, these all seem like good cleanups, so I do not see any of these changes as arguments against adding the assertion. I propose to put this in master. Patch #3 adds a test case that would have caught the bug fixed by patch #1 if we had already had the asserts added by patch #2. To my surprise, we seem to have zero existing test coverage of creating an index on a whole-row expression, so I think this is worth adding mostly for that reason. One could also argue that it's worth adding as a follow-up to #1 and #2, but we're unlikely to reintroduce that specific bug. We might, however, add other bugs that this would also catch. Comments? -- Robert Haas EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v1-0001-Prevent-spurious-indexes-on-virtual-generated-col.patch (2.2K, 2-v1-0001-Prevent-spurious-indexes-on-virtual-generated-col.patch) download | inline diff: From 36829f53acbbadb0e24254952a6d6ec036f41639 Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Fri, 20 Mar 2026 11:09:44 -0400 Subject: [PATCH v1 1/3] Prevent spurious "indexes on virtual generated columns are not supported". Both of the checks in DefineIndex() that can produce this error message have a guard against negative attribute numbers, but lack a guard to ensure that attno is non-zero. As a result, we can index off the beginning of the TupleDesc and read a garbage byte for attgenerated. If that byte happens to be 'v', we'll incorrectly produce the error mentioned above. The first call site is easy to hit: any attempt to create an expression index does so. The second one is not currently hit in the regression tests, but can be hit by something like CREATE INDEX ON some_table ((some_function(some_table))). Found by study of a test_plan_advice failure on buildfarm member skink, though this issue has nothing to do with test_plan_advice and seems to have only been revealed by happenstance. Backpatch-through: 18 --- src/backend/commands/indexcmds.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index b89c6855364..dd593ccbc1c 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1123,7 +1123,8 @@ DefineIndex(ParseState *pstate, errmsg("index creation on system columns is not supported"))); - if (TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) + if (attno > 0 && + TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) ereport(ERROR, errcode(ERRCODE_FEATURE_NOT_SUPPORTED), stmt->primary ? @@ -1164,7 +1165,8 @@ DefineIndex(ParseState *pstate, { AttrNumber attno = j + FirstLowInvalidHeapAttributeNumber; - if (TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) + if (attno > 0 && + TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), stmt->isconstraint ? -- 2.51.0 [application/octet-stream] v1-0003-Add-a-test-for-creating-an-index-on-a-whole-row-e.patch (2.6K, 3-v1-0003-Add-a-test-for-creating-an-index-on-a-whole-row-e.patch) download | inline diff: From 04e0aee0ab7cc6b05afed3fa8479d168225918bb Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Fri, 20 Mar 2026 11:36:11 -0400 Subject: [PATCH v1 3/3] Add a test for creating an index on a whole-row expression. Surprisingly, we have no existing test for this. Had this test been present before commit 0000000000000000000000000000000000000000, the Assert added in commit 0000000000000000000000000000000000000000 would have caught the bug. !!! Update this with the final commit hashes. --- src/test/regress/expected/indexing.out | 11 +++++++++++ src/test/regress/sql/indexing.sql | 12 ++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index dc629928c8f..f50868ca6a6 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -1669,3 +1669,14 @@ reindex index test_pg_index_toast_index; drop index test_pg_index_toast_index; drop function test_pg_index_toast_func; drop table test_pg_index_toast_table; +-- test creation of an index involving a whole-row expression +create table test_pg_wholerow_index (a int, b text, c numeric); +create or replace function row_image(test_pg_wholerow_index) + returns test_pg_wholerow_index as $$select $1$$ language sql immutable; +insert into test_pg_wholerow_index values (1, 'multiplication', 1.0); +create index row_image_index + on test_pg_wholerow_index ((row_image(test_pg_wholerow_index))); +insert into test_pg_wholerow_index values (2, 'addition', 0); +drop index row_image_index; +drop function row_image(test_pg_wholerow_index); +drop table test_pg_wholerow_index; diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index b5cb01c2d70..129130d04d4 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -934,3 +934,15 @@ reindex index test_pg_index_toast_index; drop index test_pg_index_toast_index; drop function test_pg_index_toast_func; drop table test_pg_index_toast_table; + +-- test creation of an index involving a whole-row expression +create table test_pg_wholerow_index (a int, b text, c numeric); +create or replace function row_image(test_pg_wholerow_index) + returns test_pg_wholerow_index as $$select $1$$ language sql immutable; +insert into test_pg_wholerow_index values (1, 'multiplication', 1.0); +create index row_image_index + on test_pg_wholerow_index ((row_image(test_pg_wholerow_index))); +insert into test_pg_wholerow_index values (2, 'addition', 0); +drop index row_image_index; +drop function row_image(test_pg_wholerow_index); +drop table test_pg_wholerow_index; -- 2.51.0 [application/octet-stream] v1-0002-Bounds-check-access-to-TupleDescAttr-with-an-Asse.patch (5.9K, 4-v1-0002-Bounds-check-access-to-TupleDescAttr-with-an-Asse.patch) download | inline diff: From bf15cdc8dc96af8c56f7a4e3ba0c49acf50f480f Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Fri, 20 Mar 2026 11:27:05 -0400 Subject: [PATCH v1 2/3] Bounds-check access to TupleDescAttr with an Assert. The second argument to TupleDescAttr should always be at least zero and less than natts; otherwise, we index outside of the attribute array. Assert that this is the case. Various violations, or possible violations, of this rule that are currently in the tree are actually harmless, because while we do call TupleDescAttr() before verifying that the argument is within range, we don't actually dereference it unless the argument was within range all along. Nonetheless, the Assert means we should be more careful, so tidy up accordingly. --- src/backend/access/common/tupdesc.c | 27 +++++++++++++++------------ src/include/access/tupdesc.h | 2 ++ src/pl/plperl/plperl.c | 7 +++++-- src/pl/plpgsql/src/pl_exec.c | 6 ++++-- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c index d771a265b34..196472c05d0 100644 --- a/src/backend/access/common/tupdesc.c +++ b/src/backend/access/common/tupdesc.c @@ -246,10 +246,11 @@ CreateTupleDescCopy(TupleDesc tupdesc) desc = CreateTemplateTupleDesc(tupdesc->natts); - /* Flat-copy the attribute array */ - memcpy(TupleDescAttr(desc, 0), - TupleDescAttr(tupdesc, 0), - desc->natts * sizeof(FormData_pg_attribute)); + /* Flat-copy the attribute array (unless there are no attributes) */ + if (desc->natts > 0) + memcpy(TupleDescAttr(desc, 0), + TupleDescAttr(tupdesc, 0), + desc->natts * sizeof(FormData_pg_attribute)); /* * Since we're not copying constraints and defaults, clear fields @@ -294,10 +295,11 @@ CreateTupleDescTruncatedCopy(TupleDesc tupdesc, int natts) desc = CreateTemplateTupleDesc(natts); - /* Flat-copy the attribute array */ - memcpy(TupleDescAttr(desc, 0), - TupleDescAttr(tupdesc, 0), - desc->natts * sizeof(FormData_pg_attribute)); + /* Flat-copy the attribute array (unless there are no attributes) */ + if (desc->natts > 0) + memcpy(TupleDescAttr(desc, 0), + TupleDescAttr(tupdesc, 0), + desc->natts * sizeof(FormData_pg_attribute)); /* * Since we're not copying constraints and defaults, clear fields @@ -339,10 +341,11 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc) desc = CreateTemplateTupleDesc(tupdesc->natts); - /* Flat-copy the attribute array */ - memcpy(TupleDescAttr(desc, 0), - TupleDescAttr(tupdesc, 0), - desc->natts * sizeof(FormData_pg_attribute)); + /* Flat-copy the attribute array (unless there are no attributes) */ + if (desc->natts > 0) + memcpy(TupleDescAttr(desc, 0), + TupleDescAttr(tupdesc, 0), + desc->natts * sizeof(FormData_pg_attribute)); for (i = 0; i < desc->natts; i++) { diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h index 62ef6b38497..d26287271e9 100644 --- a/src/include/access/tupdesc.h +++ b/src/include/access/tupdesc.h @@ -179,6 +179,8 @@ TupleDescAttr(TupleDesc tupdesc, int i) { FormData_pg_attribute *attrs = TupleDescAttrAddress(tupdesc); + Assert(i >= 0 && i < tupdesc->natts); + return &attrs[i]; } diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c index c5f11b874c7..06ebffa111c 100644 --- a/src/pl/plperl/plperl.c +++ b/src/pl/plperl/plperl.c @@ -1093,7 +1093,7 @@ plperl_build_tuple_result(HV *perlhash, TupleDesc td) SV *val = HeVAL(he); char *key = hek2cstr(he); int attn = SPI_fnumber(td, key); - Form_pg_attribute attr = TupleDescAttr(td, attn - 1); + Form_pg_attribute attr; if (attn == SPI_ERROR_NOATTRIBUTE) ereport(ERROR, @@ -1106,6 +1106,7 @@ plperl_build_tuple_result(HV *perlhash, TupleDesc td) errmsg("cannot set system attribute \"%s\"", key))); + attr = TupleDescAttr(td, attn - 1); values[attn - 1] = plperl_sv_to_datum(val, attr->atttypid, attr->atttypmod, @@ -1799,7 +1800,7 @@ plperl_modify_tuple(HV *hvTD, TriggerData *tdata, HeapTuple otup) char *key = hek2cstr(he); SV *val = HeVAL(he); int attn = SPI_fnumber(tupdesc, key); - Form_pg_attribute attr = TupleDescAttr(tupdesc, attn - 1); + Form_pg_attribute attr; if (attn == SPI_ERROR_NOATTRIBUTE) ereport(ERROR, @@ -1811,6 +1812,8 @@ plperl_modify_tuple(HV *hvTD, TriggerData *tdata, HeapTuple otup) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot set system attribute \"%s\"", key))); + + attr = TupleDescAttr(tupdesc, attn - 1); if (attr->attgenerated) ereport(ERROR, (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED), diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 84552e32c87..a3869072093 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -3369,7 +3369,7 @@ exec_stmt_return_next(PLpgSQL_execstate *estate, PLpgSQL_var *var = (PLpgSQL_var *) retvar; Datum retval = var->value; bool isNull = var->isnull; - Form_pg_attribute attr = TupleDescAttr(tupdesc, 0); + Form_pg_attribute attr; if (natts != 1) ereport(ERROR, @@ -3382,6 +3382,7 @@ exec_stmt_return_next(PLpgSQL_execstate *estate, var->datatype->typlen); /* coerce type if needed */ + attr = TupleDescAttr(tupdesc, 0); retval = exec_cast_value(estate, retval, &isNull, @@ -3500,7 +3501,7 @@ exec_stmt_return_next(PLpgSQL_execstate *estate, } else { - Form_pg_attribute attr = TupleDescAttr(tupdesc, 0); + Form_pg_attribute attr; /* Simple scalar result */ if (natts != 1) @@ -3509,6 +3510,7 @@ exec_stmt_return_next(PLpgSQL_execstate *estate, errmsg("wrong result type supplied in RETURN NEXT"))); /* coerce type if needed */ + attr = TupleDescAttr(tupdesc, 0); retval = exec_cast_value(estate, retval, &isNull, -- 2.51.0 ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TupleDescAttr bounds checks @ 2026-03-20 16:28 Robert Haas <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 2 replies; 10+ messages in thread From: Robert Haas @ 2026-03-20 16:28 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Fri, Mar 20, 2026 at 12:22 PM Tom Lane <[email protected]> wrote: > Robert Haas <[email protected]> writes: > > Scrutiny of a recent test_plan_advice failure in the buildfarm > > revealed a bug that had nothing to do with test_plan_advice or > > pg_plan_advice; rather, it was a bug introduced by the virtual > > generated columns feature, and specifically of that feature indexing > > off of the beginning of a TupleDesc when whole-row attributes are > > present. The first patch attached to this email fixes this issue, and > > should be committed and back-patched to v18. I plan to do that soon > > unless there are objections. > > I had just come to the same conclusion about why grison is failing. > +1 to all three of these patches. (I did not look to see if 0002 > fixes every case that the Assert could trigger on, but as long as > you're only putting it in HEAD I'm not too concerned that we might > have missed some.) Hmm, I had a rougher version of this analysis (and an analysis of some the other failures) on an email I sent yesterday on the pg_plan_advice thread. Based on this email and another one you sent, I'm guessing you either didn't see that email or maybe even didn't get a copy of it for some reason. Or maybe you just mean that you were checking over my analysis, but just in case: https://www.postgresql.org/message-id/CA%2BTgmoZUN8FT1Ah%3Dm6Uis5bHa4FUa%2B_hMDWtcABG17toEfpiUg%40ma... -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TupleDescAttr bounds checks @ 2026-03-20 17:02 Robert Haas <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 10+ messages in thread From: Robert Haas @ 2026-03-20 17:02 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Fri, Mar 20, 2026 at 12:46 PM Tom Lane <[email protected]> wrote: > I did see that, but it read to me that you were just guessing at that > time. This morning I put Asserts into indexcmds.c that verified that > it was trying to access the tupledesc for attno zero, and that proves > there is a bug there. It also seems like a plausible explanation for > why only one machine has exhibited the failure. (Your 0002 is a > better version of said Asserts.) Ah, OK. Yeah, I wasn't completely sure at the time whether there was some kind of TupleDesc out there that would allow zero or negative indexes safely. It seems like there is not. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TupleDescAttr bounds checks @ 2026-03-24 10:45 Robert Haas <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: Robert Haas @ 2026-03-24 10:45 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Sun, Mar 22, 2026 at 11:54 PM Tom Lane <[email protected]> wrote: > Anyway, you should get this fix pushed. Done, and I'll plan to commit the other patches later today. Also, if on any occasion you happen to feel that I'm not being aggressive enough in committing something I've previously posted, feel free to take matters into your own hands. I often wait a bit to see if anybody will object to things or comment on them, and in this case there were compounding factors like (1) the weekend and (2) being very busy looking into other problems that test_plan_advice turned up. Since this was such a simple fix and you'd +1'd it, I would have felt comfortable putting it in right away, but I simply haven't had a moment to spare until now, and I use that term loosely given that I do not normally use the time between 6am and 7am for to commit patches. Anyway, the point is: I'm virtually always happy when someone else decides to commit one of my patches; it saves me a non-trivial amount of time and I'm not offended. Thanks, -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TupleDescAttr bounds checks @ 2026-04-04 14:00 Alexander Lakhin <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Alexander Lakhin @ 2026-04-04 14:00 UTC (permalink / raw) To: Robert Haas <[email protected]>; Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Hello Robert, 24.03.2026 12:45, Robert Haas wrote: > On Sun, Mar 22, 2026 at 11:54 PM Tom Lane<[email protected]> wrote: >> Anyway, you should get this fix pushed. > Done, and I'll plan to commit the other patches later today. I've found a way to trigger the Assert added in c98ad086a: CREATE TABLE t(i int); COPY t FROM stdin WHERE tableoid > 0; server closed the connection unexpectedly (gdb) bt #0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44 #1 __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78 #2 __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89 #3 0x00007066d0e4527e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #4 0x00007066d0e288ff in __GI_abort () at ./stdlib/abort.c:79 #5 0x0000609680cc7fcf in ExceptionalCondition ( conditionName=conditionName@entry=0x609680d2c104 "i >= 0 && i < tupdesc->natts", fileName=fileName@entry=0x609680d84568 "../../../src/include/access/tupdesc.h", lineNumber=lineNumber@entry=182) at assert.c:65 #6 0x00006096808c8f68 in TupleDescAttr (tupdesc=<optimized out>, i=<optimized out>) at ../../../src/include/access/tupdesc.h:182 #7 TupleDescAttr (i=<optimized out>, tupdesc=<optimized out>) at ../../../src/include/access/tupdesc.h:178 #8 DoCopy (pstate=0x6096a749dc90, stmt=0x6096a74ca120, stmt_location=0, stmt_len=40, processed=0x7ffd3b09b450) at copy.c:180 #9 0x0000609680b6b82a in standard_ProcessUtility (pstmt=0x6096a74ca1f0, queryString=0x6096a74c9520 "COPY t FROM stdin WHERE tableoid > 0;", readOnlyTree=<optimized out>, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x6096a74ca5b0, qc=0x7ffd3b09b6f0) at utility.c:743 #10 0x0000609680b6990c in PortalRunUtility (portal=portal@entry=0x6096a756a070, pstmt=pstmt@entry=0x6096a74ca1f0, isTopLevel=isTopLevel@entry=true, setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x6096a74ca5b0, qc=qc@entry=0x7ffd3b09b6f0) at pquery.c:1148 ... Best regards, Alexander ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TupleDescAttr bounds checks @ 2026-04-04 14:30 Tom Lane <[email protected]> parent: Alexander Lakhin <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tom Lane @ 2026-04-04 14:30 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Robert Haas <[email protected]>; PostgreSQL Hackers <[email protected]> Alexander Lakhin <[email protected]> writes: > I've found a way to trigger the Assert added in c98ad086a: > CREATE TABLE t(i int); > COPY t FROM stdin WHERE tableoid > 0; > server closed the connection unexpectedly Good catch. I bet it's possible to trigger the Assert just above, too, with a WHERE expression using "t.*". regards, tom lane ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TupleDescAttr bounds checks @ 2026-04-04 14:38 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tom Lane @ 2026-04-04 14:38 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Robert Haas <[email protected]>; PostgreSQL Hackers <[email protected]> I wrote: > Good catch. I bet it's possible to trigger the Assert just above, > too, with a WHERE expression using "t.*". Oh, scratch that, I didn't read the code just above this loop. But I bet this loop should throw an error for system columns, too, since we surely won't have computed those either. regards, tom lane ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TupleDescAttr bounds checks @ 2026-04-04 15:38 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tom Lane @ 2026-04-04 15:38 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Robert Haas <[email protected]>; PostgreSQL Hackers <[email protected]> I wrote: > But I bet this loop should throw an error for system columns, too, > since we surely won't have computed those either. After poking at that: testing tableoid does sort of work, in that it reads as the OID of the target table named in COPY. But I think any rational use for a test on tableoid here would be in connection with a partitioned target table, and the user would wish it to read as the OID of the destination partition. So I think we should disallow tableoid along with the other system columns, pending somebody having the ambition to make that work. So I propose the attached for HEAD. (I couldn't resist the temptation to clean up adjacent comments.) In the back branches it might be better to just ignore system columns here, on the tiny chance that somebody thinks they do something useful. regards, tom lane Attachments: [text/x-diff] v1-reject-system-columns-in-COPY-WHERE.patch (4.6K, 2-v1-reject-system-columns-in-COPY-WHERE.patch) download | inline diff: diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index e837f417d0d..003b70852bb 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -137,22 +137,22 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, Bitmapset *expr_attrs = NULL; int i; - /* add nsitem to query namespace */ + /* Add nsitem to query namespace */ addNSItemToQuery(pstate, nsitem, false, true, true); /* Transform the raw expression tree */ whereClause = transformExpr(pstate, stmt->whereClause, EXPR_KIND_COPY_WHERE); - /* Make sure it yields a boolean result. */ + /* Make sure it yields a boolean result */ whereClause = coerce_to_boolean(pstate, whereClause, "WHERE"); - /* we have to fix its collations too */ + /* We have to fix its collations too */ assign_expr_collations(pstate, whereClause); /* - * Examine all the columns in the WHERE clause expression. When - * the whole-row reference is present, examine all the columns of - * the table. + * Identify all columns used in the WHERE clause's expression. If + * there's a whole-row reference, replace it with a range of all + * the user columns (caution: that'll include dropped columns). */ pull_varattnos(whereClause, 1, &expr_attrs); if (bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, expr_attrs)) @@ -163,12 +163,30 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, expr_attrs = bms_del_member(expr_attrs, 0 - FirstLowInvalidHeapAttributeNumber); } + /* Now we can scan each column needed in the WHERE clause */ i = -1; while ((i = bms_next_member(expr_attrs, i)) >= 0) { AttrNumber attno = i + FirstLowInvalidHeapAttributeNumber; + Form_pg_attribute att; - Assert(attno != 0); + Assert(attno != 0); /* removed above */ + + /* + * Prohibit system columns in the WHERE clause. They won't + * have been filled yet when the filtering happens. (We could + * allow tableoid, but right now it isn't really useful: it + * will read as the target table's OID. Any conceivable use + * for such a WHERE clause would probably wish it to read as + * the target partition's OID, which is not known yet. + * Disallow it to keep flexibility to change that sometime.) + */ + if (attno < 0) + ereport(ERROR, + errcode(ERRCODE_INVALID_COLUMN_REFERENCE), + errmsg("system columns are not supported in COPY FROM WHERE conditions"), + errdetail("Column \"%s\" is a system column.", + get_attname(RelationGetRelid(rel), attno, false))); /* * Prohibit generated columns in the WHERE clause. Stored @@ -177,7 +195,8 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, * would need to expand them somewhere around here), but for * now we keep them consistent with the stored variant. */ - if (TupleDescAttr(RelationGetDescr(rel), attno - 1)->attgenerated) + att = TupleDescAttr(RelationGetDescr(rel), attno - 1); + if (att->attgenerated && !att->attisdropped) ereport(ERROR, errcode(ERRCODE_INVALID_COLUMN_REFERENCE), errmsg("generated columns are not supported in COPY FROM WHERE conditions"), @@ -185,6 +204,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, get_attname(RelationGetRelid(rel), attno, false))); } + /* Reduce WHERE clause to standard list-of-AND-terms form */ whereClause = eval_const_expressions(NULL, whereClause); whereClause = (Node *) canonicalize_qual((Expr *) whereClause, false); diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index 01101c71051..7600e5239d2 100644 --- a/src/test/regress/expected/copy2.out +++ b/src/test/regress/expected/copy2.out @@ -204,6 +204,9 @@ COPY x from stdin WHERE a = row_number() over(b); ERROR: window functions are not allowed in COPY FROM WHERE conditions LINE 1: COPY x from stdin WHERE a = row_number() over(b); ^ +COPY x from stdin WHERE tableoid = 'x'::regclass; +ERROR: system columns are not supported in COPY FROM WHERE conditions +DETAIL: Column "tableoid" is a system column. -- check results of copy in SELECT * FROM x; a | b | c | d | e diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql index 889dcf1383f..e0810109473 100644 --- a/src/test/regress/sql/copy2.sql +++ b/src/test/regress/sql/copy2.sql @@ -168,6 +168,8 @@ COPY x from stdin WHERE a IN (generate_series(1,5)); COPY x from stdin WHERE a = row_number() over(b); +COPY x from stdin WHERE tableoid = 'x'::regclass; + -- check results of copy in SELECT * FROM x; ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TupleDescAttr bounds checks @ 2026-04-04 21:52 Peter Eisentraut <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Peter Eisentraut @ 2026-04-04 21:52 UTC (permalink / raw) To: Tom Lane <[email protected]>; Alexander Lakhin <[email protected]>; +Cc: Robert Haas <[email protected]>; PostgreSQL Hackers <[email protected]> On 04.04.26 17:38, Tom Lane wrote: > I wrote: >> But I bet this loop should throw an error for system columns, too, >> since we surely won't have computed those either. > > After poking at that: testing tableoid does sort of work, in that it > reads as the OID of the target table named in COPY. But I think any > rational use for a test on tableoid here would be in connection with > a partitioned target table, and the user would wish it to read as the > OID of the destination partition. So I think we should disallow > tableoid along with the other system columns, pending somebody having > the ambition to make that work. > > So I propose the attached for HEAD. (I couldn't resist the temptation > to clean up adjacent comments.) In the back branches it might be > better to just ignore system columns here, on the tiny chance that > somebody thinks they do something useful. I think this is the same issue that was discussed here: https://www.postgresql.org/message-id/flat/30c39ee8-bb11-4b8f-9697-45f7e018a8d3%40eisentraut.org There was no conclusion there, but I agree with the proposal to prohibit this use. ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TupleDescAttr bounds checks @ 2026-04-05 03:45 Tom Lane <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Tom Lane @ 2026-04-05 03:45 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Robert Haas <[email protected]>; PostgreSQL Hackers <[email protected]> Peter Eisentraut <[email protected]> writes: > On 04.04.26 17:38, Tom Lane wrote: >> After poking at that: testing tableoid does sort of work, in that it >> reads as the OID of the target table named in COPY. But I think any >> rational use for a test on tableoid here would be in connection with >> a partitioned target table, and the user would wish it to read as the >> OID of the destination partition. So I think we should disallow >> tableoid along with the other system columns, pending somebody having >> the ambition to make that work. > I think this is the same issue that was discussed here: > https://www.postgresql.org/message-id/flat/30c39ee8-bb11-4b8f-9697-45f7e018a8d3%40eisentraut.org > There was no conclusion there, but I agree with the proposal to prohibit > this use. Ah, indeed. Jian's patch in that thread seems rough but potentially workable to me, but seemingly the thread tailed off for lack of interest. I don't want to revive it now as part of a bug fix. Disallowing tableoid for now, and then re-allowing it if someone picks up that patch down the road, seems like a good solution. For one thing, since that patch changes the semantics of tableoid in COPY WHERE, I think it'd be a good idea to have a release or two in between where we throw error. That'd be helpful to flush out any field usages that might be affected. regards, tom lane ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2026-04-05 03:45 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-03-20 15:58 TupleDescAttr bounds checks Robert Haas <[email protected]> 2026-03-20 16:28 ` Robert Haas <[email protected]> 2026-03-20 17:02 ` Robert Haas <[email protected]> 2026-03-24 10:45 ` Robert Haas <[email protected]> 2026-04-04 14:00 ` Alexander Lakhin <[email protected]> 2026-04-04 14:30 ` Tom Lane <[email protected]> 2026-04-04 14:38 ` Tom Lane <[email protected]> 2026-04-04 15:38 ` Tom Lane <[email protected]> 2026-04-04 21:52 ` Peter Eisentraut <[email protected]> 2026-04-05 03:45 ` 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