Hi Jian!
The feature makes sense from my POV.
> On 31 Dec 2025, at 10:08, jian he <jian.universality@gmail.com> 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
withresolved 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
threaddoes 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.
