public inbox for [email protected]help / color / mirror / Atom feed
SPLIT/MERGE use of is_internal=true 6+ messages / 2 participants [nested] [flat]
* SPLIT/MERGE use of is_internal=true @ 2026-07-07 18:57 Noah Misch <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Noah Misch @ 2026-07-07 18:57 UTC (permalink / raw) To: [email protected]; [email protected]; +Cc: pgsql-hackers commit f2e4cc4 wrote: > +/* > + * createPartitionTable: > + * > + * Create a new partition (newPartName) for the partitioned table (parent_rel). > + * ownerId is determined by the partition on which the operation is performed, > + * so it is passed separately. The new partition will inherit the access method > + * and persistence type from the parent table. > + * > + * Returns the created relation (locked in AccessExclusiveLock mode). > + */ > +static Relation > +createPartitionTable(List **wqueue, RangeVar *newPartName, > + Relation parent_rel, Oid ownerId) > +{ ... > + /* Create the relation. */ > + newRelId = heap_create_with_catalog(newPartName->relname, > + namespaceId, > + parent_relform->reltablespace, > + InvalidOid, > + InvalidOid, > + InvalidOid, > + ownerId, > + relamId, > + descriptor, > + NIL, > + RELKIND_RELATION, > + newPartName->relpersistence, > + false, > + false, > + ONCOMMIT_NOOP, > + (Datum) 0, > + true, > + allowSystemTableMods, > + true, > + InvalidOid, > + NULL); This and other places in the SPLIT/MERGE patches pass is_internal=true, which ultimately flows to hooks with the following meaning: /* * If this flag is set, the user hasn't requested that the object be * altered, but we're doing it anyway for some internal reason. * Permissions-checking hooks may want to skip checks if, say, we're alter * the constraints of a temporary heap during CLUSTER. */ bool is_internal; SPLIT/MERGE constitute user requests to create and/or drop tables, so they're not like a CLUSTER temporary heap. They should pass is_internal=false, like when the user runs CREATE/DROP directly. ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SPLIT/MERGE use of is_internal=true @ 2026-07-07 23:49 Alexander Korotkov <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Alexander Korotkov @ 2026-07-07 23:49 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: [email protected]; pgsql-hackers On Tue, Jul 7, 2026 at 9:57 PM Noah Misch <[email protected]> wrote: > > commit f2e4cc4 wrote: > > +/* > > + * createPartitionTable: > > + * > > + * Create a new partition (newPartName) for the partitioned table (parent_rel). > > + * ownerId is determined by the partition on which the operation is performed, > > + * so it is passed separately. The new partition will inherit the access method > > + * and persistence type from the parent table. > > + * > > + * Returns the created relation (locked in AccessExclusiveLock mode). > > + */ > > +static Relation > > +createPartitionTable(List **wqueue, RangeVar *newPartName, > > + Relation parent_rel, Oid ownerId) > > +{ > ... > > + /* Create the relation. */ > > + newRelId = heap_create_with_catalog(newPartName->relname, > > + namespaceId, > > + parent_relform->reltablespace, > > + InvalidOid, > > + InvalidOid, > > + InvalidOid, > > + ownerId, > > + relamId, > > + descriptor, > > + NIL, > > + RELKIND_RELATION, > > + newPartName->relpersistence, > > + false, > > + false, > > + ONCOMMIT_NOOP, > > + (Datum) 0, > > + true, > > + allowSystemTableMods, > > + true, > > + InvalidOid, > > + NULL); > > This and other places in the SPLIT/MERGE patches pass is_internal=true, which > ultimately flows to hooks with the following meaning: > > /* > * If this flag is set, the user hasn't requested that the object be > * altered, but we're doing it anyway for some internal reason. > * Permissions-checking hooks may want to skip checks if, say, we're alter > * the constraints of a temporary heap during CLUSTER. > */ > bool is_internal; > > SPLIT/MERGE constitute user requests to create and/or drop tables, so they're > not like a CLUSTER temporary heap. They should pass is_internal=false, like > when the user runs CREATE/DROP directly. Thank you for catching this. For sure, we shouldn't pass is_internal=true here. Attached patch fixes this. I'm going to push it if no objections. ------ Regards, Alexander Korotkov Supabase Attachments: [application/octet-stream] v1-0001-Don-t-create-SPLIT-MERGE-partitions-as-internal-r.patch (1.4K, ../../CAPpHfdsVnD=BNHFo37sUkMCBi5EpsSJA3DYRGn5rOSdpVonpuA@mail.gmail.com/2-v1-0001-Don-t-create-SPLIT-MERGE-partitions-as-internal-r.patch) download | inline diff: From 775398f6823378a682ec5347884fd4a9390d9679 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Wed, 8 Jul 2026 02:47:07 +0300 Subject: [PATCH v1] Don't create SPLIT/MERGE partitions as internal relations createPartitionTable(), which builds the new partitions for ALTER TABLE ... SPLIT PARTITION and ALTER TABLE ... MERGE PARTITIONS, passed is_internal=true to heap_create_with_catalog(). These relations are created at the explicit request of the user, just like a plain CREATE TABLE, so pass is_internal=false instead. The is_internal flag is meant for objects created as an internal implementation detail (for example, a transient heap built during CLUSTER), and it is observed by object-access hooks. Reported-by: Noah Misch <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/commands/tablecmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 95abaf4890c..0b6a84b32b7 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -23137,7 +23137,7 @@ createPartitionTable(List **wqueue, RangeVar *newPartName, (Datum) 0, true, allowSystemTableMods, - true, + false, /* is_internal */ InvalidOid, NULL); -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SPLIT/MERGE use of is_internal=true @ 2026-07-07 23:58 Noah Misch <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Noah Misch @ 2026-07-07 23:58 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: [email protected]; pgsql-hackers On Wed, Jul 08, 2026 at 02:49:45AM +0300, Alexander Korotkov wrote: > On Tue, Jul 7, 2026 at 9:57 PM Noah Misch <[email protected]> wrote: > > This and other places in the SPLIT/MERGE patches pass is_internal=true, which > > ultimately flows to hooks with the following meaning: > > > > /* > > * If this flag is set, the user hasn't requested that the object be > > * altered, but we're doing it anyway for some internal reason. > > * Permissions-checking hooks may want to skip checks if, say, we're alter > > * the constraints of a temporary heap during CLUSTER. > > */ > > bool is_internal; > > > > SPLIT/MERGE constitute user requests to create and/or drop tables, so they're > > not like a CLUSTER temporary heap. They should pass is_internal=false, like > > when the user runs CREATE/DROP directly. > > Thank you for catching this. For sure, we shouldn't pass > is_internal=true here. Attached patch fixes this. I'm going to push > it if no objections. > Subject: [PATCH v1] Don't create SPLIT/MERGE partitions as internal relations > > createPartitionTable(), which builds the new partitions for ALTER TABLE > ... SPLIT PARTITION and ALTER TABLE ... MERGE PARTITIONS, passed > is_internal=true to heap_create_with_catalog(). These relations are > created at the explicit request of the user, just like a plain CREATE > TABLE, so pass is_internal=false instead. The is_internal flag is meant > for objects created as an internal implementation detail (for example, a > transient heap built during CLUSTER), and it is observed by > object-access hooks. > > Reported-by: Noah Misch <[email protected]> > Discussion: https://postgr.es/m/[email protected] Log should have a "Backpatch-through: 19" line. > --- > src/backend/commands/tablecmds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c > index 95abaf4890c..0b6a84b32b7 100644 > --- a/src/backend/commands/tablecmds.c > +++ b/src/backend/commands/tablecmds.c > @@ -23137,7 +23137,7 @@ createPartitionTable(List **wqueue, RangeVar *newPartName, > (Datum) 0, > true, > allowSystemTableMods, > - true, > + false, /* is_internal */ > InvalidOid, > NULL); The edit looks fine for this code site. As I mentioned above, other SPLIT/MERGE code sites need the same change. ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SPLIT/MERGE use of is_internal=true @ 2026-07-08 00:18 Alexander Korotkov <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Alexander Korotkov @ 2026-07-08 00:18 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: [email protected]; pgsql-hackers On Wed, Jul 8, 2026 at 2:58 AM Noah Misch <[email protected]> wrote: > > On Wed, Jul 08, 2026 at 02:49:45AM +0300, Alexander Korotkov wrote: > > On Tue, Jul 7, 2026 at 9:57 PM Noah Misch <[email protected]> wrote: > > > This and other places in the SPLIT/MERGE patches pass is_internal=true, which > > > ultimately flows to hooks with the following meaning: > > > > > > /* > > > * If this flag is set, the user hasn't requested that the object be > > > * altered, but we're doing it anyway for some internal reason. > > > * Permissions-checking hooks may want to skip checks if, say, we're alter > > > * the constraints of a temporary heap during CLUSTER. > > > */ > > > bool is_internal; > > > > > > SPLIT/MERGE constitute user requests to create and/or drop tables, so they're > > > not like a CLUSTER temporary heap. They should pass is_internal=false, like > > > when the user runs CREATE/DROP directly. > > > > Thank you for catching this. For sure, we shouldn't pass > > is_internal=true here. Attached patch fixes this. I'm going to push > > it if no objections. > > > Subject: [PATCH v1] Don't create SPLIT/MERGE partitions as internal relations > > > > createPartitionTable(), which builds the new partitions for ALTER TABLE > > ... SPLIT PARTITION and ALTER TABLE ... MERGE PARTITIONS, passed > > is_internal=true to heap_create_with_catalog(). These relations are > > created at the explicit request of the user, just like a plain CREATE > > TABLE, so pass is_internal=false instead. The is_internal flag is meant > > for objects created as an internal implementation detail (for example, a > > transient heap built during CLUSTER), and it is observed by > > object-access hooks. > > > > Reported-by: Noah Misch <[email protected]> > > Discussion: https://postgr.es/m/[email protected] > > Log should have a "Backpatch-through: 19" line. Thank you for catching. > > --- > > src/backend/commands/tablecmds.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c > > index 95abaf4890c..0b6a84b32b7 100644 > > --- a/src/backend/commands/tablecmds.c > > +++ b/src/backend/commands/tablecmds.c > > @@ -23137,7 +23137,7 @@ createPartitionTable(List **wqueue, RangeVar *newPartName, > > (Datum) 0, > > true, > > allowSystemTableMods, > > - true, > > + false, /* is_internal */ > > InvalidOid, > > NULL); > > The edit looks fine for this code site. As I mentioned above, other > SPLIT/MERGE code sites need the same change. This is the only call heap_create_with_catalog() using in SPLIT/MERGE code. But now I see there are calls of other catalog functions with is_internal == true. I'll check it more precisely tomorrow. ------ Regards, Alexander Korotkov Supabase ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SPLIT/MERGE use of is_internal=true @ 2026-07-08 18:32 Alexander Korotkov <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Alexander Korotkov @ 2026-07-08 18:32 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: [email protected]; pgsql-hackers On Wed, Jul 8, 2026 at 3:18 AM Alexander Korotkov <[email protected]> wrote: > On Wed, Jul 8, 2026 at 2:58 AM Noah Misch <[email protected]> wrote: > > > > On Wed, Jul 08, 2026 at 02:49:45AM +0300, Alexander Korotkov wrote: > > > On Tue, Jul 7, 2026 at 9:57 PM Noah Misch <[email protected]> wrote: > > > > This and other places in the SPLIT/MERGE patches pass is_internal=true, which > > > > ultimately flows to hooks with the following meaning: > > > > > > > > /* > > > > * If this flag is set, the user hasn't requested that the object be > > > > * altered, but we're doing it anyway for some internal reason. > > > > * Permissions-checking hooks may want to skip checks if, say, we're alter > > > > * the constraints of a temporary heap during CLUSTER. > > > > */ > > > > bool is_internal; > > > > > > > > SPLIT/MERGE constitute user requests to create and/or drop tables, so they're > > > > not like a CLUSTER temporary heap. They should pass is_internal=false, like > > > > when the user runs CREATE/DROP directly. > > > > > > Thank you for catching this. For sure, we shouldn't pass > > > is_internal=true here. Attached patch fixes this. I'm going to push > > > it if no objections. > > > > > Subject: [PATCH v1] Don't create SPLIT/MERGE partitions as internal relations > > > > > > createPartitionTable(), which builds the new partitions for ALTER TABLE > > > ... SPLIT PARTITION and ALTER TABLE ... MERGE PARTITIONS, passed > > > is_internal=true to heap_create_with_catalog(). These relations are > > > created at the explicit request of the user, just like a plain CREATE > > > TABLE, so pass is_internal=false instead. The is_internal flag is meant > > > for objects created as an internal implementation detail (for example, a > > > transient heap built during CLUSTER), and it is observed by > > > object-access hooks. > > > > > > Reported-by: Noah Misch <[email protected]> > > > Discussion: https://postgr.es/m/[email protected] > > > > Log should have a "Backpatch-through: 19" line. > > Thank you for catching. > > > > --- > > > src/backend/commands/tablecmds.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c > > > index 95abaf4890c..0b6a84b32b7 100644 > > > --- a/src/backend/commands/tablecmds.c > > > +++ b/src/backend/commands/tablecmds.c > > > @@ -23137,7 +23137,7 @@ createPartitionTable(List **wqueue, RangeVar *newPartName, > > > (Datum) 0, > > > true, > > > allowSystemTableMods, > > > - true, > > > + false, /* is_internal */ > > > InvalidOid, > > > NULL); > > > > The edit looks fine for this code site. As I mentioned above, other > > SPLIT/MERGE code sites need the same change. > > This is the only call heap_create_with_catalog() using in SPLIT/MERGE > code. But now I see there are calls of other catalog functions with > is_internal == true. I'll check it more precisely tomorrow. The attached patch removes is_internal = true from 3 other function calls, and removes PERFORM_DELETION_INTERNAL flag from performDeletionCheck() (the flag in ignored, no functional effect but code consistency). This is all I managed to find. What do you think? ------ Regards, Alexander Korotkov Supabase Attachments: [application/octet-stream] v2-0001-Don-t-create-SPLIT-MERGE-partitions-as-internal-r.patch (3.6K, ../../CAPpHfdu05sNB4M2N8Rq4ov_G3fyaMRJnCwkmByzwP4YDS8pCSg@mail.gmail.com/2-v2-0001-Don-t-create-SPLIT-MERGE-partitions-as-internal-r.patch) download | inline diff: From 2503fc2aa262c3def2fc04c358f28a63bd31dc2a Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Wed, 8 Jul 2026 21:23:15 +0300 Subject: [PATCH v2] Don't create SPLIT/MERGE partitions as internal relations The new partitions built for ALTER TABLE ... SPLIT PARTITION and ALTER TABLE ... MERGE PARTITIONS are created at the explicit request of the user, just like a plain CREATE TABLE. createPartitionTable() passes is_internal=true to heap_create_with_catalog(), while createTableConstraints() does the same to StoreAttrDefault() and AddRelationNewConstraints(). Pass is_internal=false in all these places instead, so that object-access hooks treat them as user-requested objects. The is_internal flag is intended for objects created as internal implementation details, such as a transient heap built during CLUSTER. While at it, pass 0 rather than PERFORM_DELETION_INTERNAL to the performDeletionCheck() calls that pre-check the drop eligibility of the old partitions, to match the subsequent performDeletion(). The flag has no functional effect on performDeletionCheck(), but change this for code consistency. Reported-by: Noah Misch <[email protected]> Discussion: https://postgr.es/m/[email protected] Backpatch-through: 19 --- src/backend/commands/tablecmds.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b116e70a4bf..cb93c3e935a 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -23063,7 +23063,7 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, elog(ERROR, "cannot convert whole-row table reference"); /* Add a pre-cooked default expression. */ - StoreAttrDefault(newRel, num, def, true); + StoreAttrDefault(newRel, num, def, false); /* * Stored generated column expressions in parent_rel might @@ -23131,7 +23131,7 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, /* Install all CHECK constraints. */ cookedConstraints = AddRelationNewConstraints(newRel, NIL, constraints, - false, true, true, NULL); + false, true, false, NULL); /* Make the additional catalog changes visible. */ CommandCounterIncrement(); @@ -23193,7 +23193,7 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, * We already set pg_attribute.attnotnull in createPartitionTable. No * need call set_attnotnull again. */ - AddRelationNewConstraints(newRel, NIL, nnconstraints, false, true, true, NULL); + AddRelationNewConstraints(newRel, NIL, nnconstraints, false, true, false, NULL); } } @@ -23313,7 +23313,7 @@ createPartitionTable(List **wqueue, RangeVar *newPartName, (Datum) 0, true, allowSystemTableMods, - true, + false, /* is_internal */ InvalidOid, NULL); @@ -23885,7 +23885,7 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel, object.classId = RelationRelationId; object.objectSubId = 0; - performDeletionCheck(&object, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); + performDeletionCheck(&object, DROP_RESTRICT, 0); } /* @@ -24299,7 +24299,7 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo *tab, Relation rel, object.objectId = splitRelOid; object.classId = RelationRelationId; object.objectSubId = 0; - performDeletionCheck(&object, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); + performDeletionCheck(&object, DROP_RESTRICT, 0); /* * If a new partition has the same name as the split partition, then we -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: SPLIT/MERGE use of is_internal=true @ 2026-07-08 22:50 Noah Misch <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Noah Misch @ 2026-07-08 22:50 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: [email protected]; pgsql-hackers On Wed, Jul 08, 2026 at 09:32:06PM +0300, Alexander Korotkov wrote: > The attached patch removes is_internal = true from 3 other function That covers all I saw. > calls, and removes PERFORM_DELETION_INTERNAL flag from > performDeletionCheck() (the flag in ignored, no functional effect but > code consistency). This is all I managed to find. What do you think? The patch looks good. The PERFORM_DELETION_INTERNAL removals make sense to me, too. ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-07-08 22:50 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-07-07 18:57 SPLIT/MERGE use of is_internal=true Noah Misch <[email protected]> 2026-07-07 23:49 ` Alexander Korotkov <[email protected]> 2026-07-07 23:58 ` Noah Misch <[email protected]> 2026-07-08 00:18 ` Alexander Korotkov <[email protected]> 2026-07-08 18:32 ` Alexander Korotkov <[email protected]> 2026-07-08 22:50 ` Noah Misch <[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