public inbox for [email protected]
help / color / mirror / Atom feedRe: CREATE TABLE LIKE INCLUDING TRIGGERS
2+ messages / 2 participants
[nested] [flat]
* Re: CREATE TABLE LIKE INCLUDING TRIGGERS
@ 2026-01-02 09:25 Andrey Borodin <[email protected]>
0 siblings, 1 reply; 2+ messages in thread
From: Andrey Borodin @ 2026-01-02 09:25 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: pgsql-hackers
Hi Jian!
The feature makes sense from my POV.
> On 31 Dec 2025, at 10:08, jian he <[email protected]> wrote:
>
> <v4-0001-add-relOid-field-to-CreateTrigStmt.patch><v4-0002-add-constrrelOid-field-to-CreateTrigStmt.patch>
I'm not an expert in the area, but still decided to review the patch a bit.
First two steps seems to add Oids along with RangeVars. It seems suspicious to me.
Parse Nodes seem to use "textual" identifiers without resolving them to Oids. Oids are specific to session catalog snapshot, but parse nodes
By adding Oid fields we will have to check both RangeVars and Oids all over the code.
Other INCLUDING options (indexes, constraints) don't modify their statement nodes this way - they create fresh nodes with resolved references.
I'm not opposed, but I suggest you to get an opinion of an expert in parse nodes about it, maybe in Discord if this thread does not attract attention. It seems a fundamental stuff for two of your patchsets.
+ char *trigcomment; /* comment to apply to trigger, or NULL */
No other Create*Stmt has a comment field. Comments seem to be handled through separate CommentStmt creation.
Some nitpicking about tests:
1. INSTEAD OF triggers on views - The error is tested, but should also test that statement-level VIEW triggers work
2. Triggers on partitioned tables - What happens when you LIKE a partitioned table? Are partition triggers cloned?
3. Cross-schema trigger functions - The function name reconstruction handles schemas, but is it tested?
+ funcname = list_make2(makeString(schemaname),makeString(NameStr(procform->proname)));
Other NameStr() are pstrdup()'d, maybe let's pstrdup() this too?
+ /* Reconstruct trigger old transition table */
Second instance of this comment is wrong.
+ PG_KEYWORD("triggers", TRIGGERS, UNRESERVED_KEYWORD, BARE_LABEL)
Won't this break some user SQLs?
Thanks!
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: CREATE TABLE LIKE INCLUDING TRIGGERS
@ 2026-01-13 18:03 Robert Haas <[email protected]>
parent: Andrey Borodin <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Robert Haas @ 2026-01-13 18:03 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: jian he <[email protected]>; pgsql-hackers
On Fri, Jan 2, 2026 at 4:25 AM Andrey Borodin <[email protected]> wrote:
> First two steps seems to add Oids along with RangeVars. It seems suspicious to me.
> Parse Nodes seem to use "textual" identifiers without resolving them to Oids. Oids are specific to session catalog snapshot, but parse nodes
> By adding Oid fields we will have to check both RangeVars and Oids all over the code.
> Other INCLUDING options (indexes, constraints) don't modify their statement nodes this way - they create fresh nodes with resolved references.
It seems like what generateClonedIndexStmt() does for the relation name is just:
index->relation = heapRel;
This is not a "resolved reference", which I would understand to mean
an OID in this context. In fact, if heapRel->schemaname were ever NULL
here, this would be at least a bug and possibly a security issue.
However, I don't think that ever happens. ProcessUtilitySlow() calls
transformCreateStmt(), which forces the RangeVar to be
fully-qualified, and then ProcessUtilitySlow() fishes out the
transformed RangeVar so that it gets passed through to
expandTableLikeClause(), which in turn passes it through to
generateClonedIndexStmt() and generateClonedExtStatsStmt(). Similarly
constraints are handled by using that same transformed (i.e. forcibly
schema-qualified) RangeVar in the AlterTableStmt we construct.
To be honest, I find this all incredibly ugly. I think that
translating from names to OIDs and back to names so that we can later
translate back to OIDs is a horrendous, bug-prone mess. Nor is the
problem confined to table names. For example,
generateClonedIndexStmt() needs to translate the AM name and
tablespace name and any column names back to textual format so that
when we actually create the index we can redo the translation in the
forward direction and hopefully get the right answer.
But having said that it's incredibly ugly and I don't like anything
about it, it also does seem to be the established pattern. As you say,
it seems as though the patch makes CREATE LIKE ... (INCLUDING
TRIGGERS) work differently from other cases, and I'm guessing that's
not what we want to do. It's better to use the same ugly hack
consistently than to use different ugly hacks in different places.
Then if somebody ever decides to try to clean this up, they can clean
up all the cases in the same way.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-01-13 18:03 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-01-02 09:25 Re: CREATE TABLE LIKE INCLUDING TRIGGERS Andrey Borodin <[email protected]>
2026-01-13 18:03 ` Robert Haas <[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