BUG #16272: Index expression can refer to wrong attributes if index is created via CREATE TABLE LIKE

PG Bug reporting form <noreply@postgresql.org> writes:

The index expression in the index created via LIKE ... INCLUDING INDEXES
still refers to the first two attributes of the table, although an attribute
has been put in place before the columns the expression referred to in the
original index.

Ugh. So the problem here is that transformTableLikeClause carefully
renumbers the Vars in the index expression to match the new column
numbers ... as they stand when it runs, which is before any account
has been taken of inheritance. It looks like Vars in check constraints
are likewise misprocessed, and probably GENERATED expressions as well.

I think this is basically another instance of the ALTER TABLE issues
I recently fixed: doing this sort of transformation at parse time is
fundamentally broken. We should refrain from trying to import the LIKE
table's indexes etc. until after MergeAttributes has done its work, and
most likely ought to just punt LIKE transformation into DefineRelation
altogether. That's probably too big a change to consider back-patching,
unfortunately.

For future reference, there are some test cases in create_table_like.sql
that come oh so close to exposing these issues. But not close enough.
See attached test-case patch (with wrong results).

regards, tom lane

I wrote:

PG Bug reporting form <noreply@postgresql.org> writes:

The index expression in the index created via LIKE ... INCLUDING INDEXES
still refers to the first two attributes of the table, although an attribute
has been put in place before the columns the expression referred to in the
original index.

Ugh. So the problem here is that transformTableLikeClause carefully
renumbers the Vars in the index expression to match the new column
numbers ... as they stand when it runs, which is before any account
has been taken of inheritance. It looks like Vars in check constraints
are likewise misprocessed, and probably GENERATED expressions as well.

I did some more investigation of this today. There are at least three
related bugs in this vicinity:

* Vars in CHECK constraints coming from LIKE ... INCLUDING CONSTRAINTS
are not renumbered when they need to be.

* Vars in GENERATED expressions coming from LIKE ... INCLUDING GENERATED
are not renumbered when they need to be.

* Vars in GENERATED expressions coming from inheritance parents
are not renumbered when they need to be.

The attached proposed patch fixes those three things. But it fails
to fix the initially-complained-of problem with indexes, because the
indexes are not part of the CreateStmt data structure received by
DefineRelation. Still, this is progress towards that, because at
least we are now building an AttrMap that would be appropriate to use
for that purpose. The remaining problem is to be able to apply that
AttrMap to the IndexStmt(s).

One idea is to pass in the list of statements generated by
transformCreateStmt to DefineRelation and let it hack on them.
That seems like an enormous violation of modularity, though.
Another, perhaps marginally cleaner, answer is to pass back
the AttrMap and let ProcessUtility hack up the IndexStmts
as it loops over the list.

I can't avoid the impression that we ought to rewrite all this logic
... DefineRelation was rather ugly even when we got it from Berkeley,
and we've layered more ugliness on it over the years. But I'm not
sure offhand what a nicer design would look like.

regards, tom lane

I wrote:

I did some more investigation of this today. There are at least three
related bugs in this vicinity:
* Vars in CHECK constraints coming from LIKE ... INCLUDING CONSTRAINTS
are not renumbered when they need to be.
* Vars in GENERATED expressions coming from LIKE ... INCLUDING GENERATED
are not renumbered when they need to be.
* Vars in GENERATED expressions coming from inheritance parents
are not renumbered when they need to be.
The attached proposed patch fixes those three things. But it fails
to fix the initially-complained-of problem with indexes, because the
indexes are not part of the CreateStmt data structure received by
DefineRelation. Still, this is progress towards that, because at
least we are now building an AttrMap that would be appropriate to use
for that purpose. The remaining problem is to be able to apply that
AttrMap to the IndexStmt(s).
One idea is to pass in the list of statements generated by
transformCreateStmt to DefineRelation and let it hack on them.
That seems like an enormous violation of modularity, though.
Another, perhaps marginally cleaner, answer is to pass back
the AttrMap and let ProcessUtility hack up the IndexStmts
as it loops over the list.

I thought of a somewhat less messy answer, which is to divide the
existing transformTableLikeClause processing into two phases, one of
which is delayed until after we've performed DefineRelation. At that
point it's no problem to convert everything from the LIKE parent
table's attnums to the new child's. MergeAttributes still has to
fix up GENERATED expressions passed down from traditional-inheritance
parents, but that does not require abusing its API like the previous
patch did.

So attached is a complete fix for the issues discussed in this thread.

Although I originally felt that we weren't going to be able to create
a back-patchable fix, this seems much less invasive than my prior
attempt, so I think it might be reasonable to back-patch. The only
external API break is the new AT_CookedColumnDefault subcommand,
which we could stick at the end of the enum in stable branches.

Thoughts?

regards, tom lane

On Thu, Aug 20, 2020 at 03:18:24PM -0400, Tom Lane wrote:

So attached is a complete fix for the issues discussed in this thread.

Although I originally felt that we weren't going to be able to create
a back-patchable fix, this seems much less invasive than my prior
attempt, so I think it might be reasonable to back-patch. The only
external API break is the new AT_CookedColumnDefault subcommand,
which we could stick at the end of the enum in stable branches.

Thoughts?

For the archive's sake, this has been committed as of 5028981,
backpatched down to 9.5.
--
Michael

Hello hackers,

22.08.2020 04:41, Michael Paquier wrote:

For the archive's sake, this has been committed as of 5028981,
backpatched down to 9.5.
--
Michael

I've encountered an issue (on REL_13_STABLE) that is attributed by `git
bisect` to 894f5dea (5028981 on master).
When executing the following query:
CREATE TABLE test(i int CHECK (i > 0));
CREATE TABLE pg_user(LIKE test INCLUDING CONSTRAINTS);

I get an assertion:
Core was generated by `postgres: law regression [local] CREATE
TABLE�������������������������������� '.
Program terminated with signal SIGABRT, Aborted.
#0� __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50����� ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0� __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1� 0x00007f8872deb859 in __GI_abort () at abort.c:79
#2� 0x0000561be6ee042b in ExceptionalCondition (
��� conditionName=conditionName@entry=0x561be6f3f120 "lockmode != NoLock
|| IsBootstrapProcessingMode() || CheckRelationLockedByMe(r,
AccessShareLock, true)", errorType=errorType@entry=0x561be6f3d01d
"FailedAssertion",
��� fileName=fileName@entry=0x561be6fe843f "relation.c",
lineNumber=lineNumber@entry=68) at assert.c:67
#3� 0x0000561be6a1e349 in relation_open (relationId=12098,
lockmode=<optimized out>) at relation.c:68
#4� 0x0000561be6b6b174 in expandTableLikeClause (heapRel=0x561be8f56e98,
��� table_like_clause=table_like_clause@entry=0x561be8f77440) at
parse_utilcmd.c:1179
#5� 0x0000561be6dafa90 in ProcessUtilitySlow
(pstate=pstate@entry=0x561be8f771c0, pstmt=pstmt@entry=0x561be8f57120,
��� queryString=queryString@entry=0x561be8f56460 "CREATE TABLE
pg_user(LIKE test INCLUDING CONSTRAINTS);",
��� context=context@entry=PROCESS_UTILITY_TOPLEVEL,
params=params@entry=0x0, queryEnv=queryEnv@entry=0x0,
��� qc=0x7ffe509d76b0, dest=<optimized out>) at utility.c:1212
#6� 0x0000561be6daec91 in standard_ProcessUtility (pstmt=0x561be8f57120,
��� queryString=0x561be8f56460 "CREATE TABLE pg_user(LIKE test INCLUDING
CONSTRAINTS);",
��� context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
dest=0x561be8f57400, qc=0x7ffe509d76b0)
��� at utility.c:1069
#7� 0x0000561be6dac066 in PortalRunUtility (portal=0x561be8fb77e0,
pstmt=0x561be8f57120, isTopLevel=<optimized out>,
��� setHoldSnapshot=<optimized out>, dest=0x561be8f57400,
qc=0x7ffe509d76b0) at pquery.c:1157
#8� 0x0000561be6dacb34 in PortalRunMulti
(portal=portal@entry=0x561be8fb77e0, isTopLevel=isTopLevel@entry=true,
��� setHoldSnapshot=setHoldSnapshot@entry=false,
dest=dest@entry=0x561be8f57400, altdest=altdest@entry=0x561be8f57400,
��� qc=qc@entry=0x7ffe509d76b0) at pquery.c:1303
#9� 0x0000561be6dadabe in PortalRun (portal=portal@entry=0x561be8fb77e0,
count=count@entry=9223372036854775807,
��� isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
dest=dest@entry=0x561be8f57400,
��� altdest=altdest@entry=0x561be8f57400, qc=0x7ffe509d76b0) at pquery.c:779
#10 0x0000561be6da905a in exec_simple_query (
��� query_string=0x561be8f56460 "CREATE TABLE pg_user(LIKE test
INCLUDING CONSTRAINTS);") at postgres.c:1239
#11 0x0000561be6daaedc in PostgresMain (argc=<optimized out>,
argv=argv@entry=0x561be8f81868, dbname=<optimized out>,
��� username=<optimized out>) at postgres.c:4315
#12 0x0000561be6d182cf in BackendRun (port=0x561be8f77ea0) at
postmaster.c:4536
#13 BackendStartup (port=0x561be8f77ea0) at postmaster.c:4220
#14 ServerLoop () at postmaster.c:1739
#15 0x0000561be6d1921f in PostmasterMain (argc=<optimized out>,
argv=0x561be8f508c0) at postmaster.c:1412
#16 0x0000561be6a0e1f2 in main (argc=3, argv=0x561be8f508c0) at main.c:210

Best regards,
Alexander

Alexander Lakhin <exclusion@gmail.com> writes:

When executing the following query:
CREATE TABLE test(i int CHECK (i > 0));
CREATE TABLE pg_user(LIKE test INCLUDING CONSTRAINTS);
I get an assertion:

Fixed, thanks for the report!

regards, tom lane