Thread
Commits
-
Implement ALTER TABLE ... MERGE PARTITIONS ... command
- f2e4cc427951 19 (unreleased) cited
-
SPLIT/MERGE use of is_internal=true
Noah Misch <noah@leadboat.com> — 2026-07-07T18:57:51Z
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. -
Re: SPLIT/MERGE use of is_internal=true
Alexander Korotkov <aekorotkov@gmail.com> — 2026-07-07T23:49:45Z
On Tue, Jul 7, 2026 at 9:57 PM Noah Misch <noah@leadboat.com> 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 -
Re: SPLIT/MERGE use of is_internal=true
Noah Misch <noah@leadboat.com> — 2026-07-07T23:58:39Z
On Wed, Jul 08, 2026 at 02:49:45AM +0300, Alexander Korotkov wrote: > On Tue, Jul 7, 2026 at 9:57 PM Noah Misch <noah@leadboat.com> 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 <noah@leadboat.com> > Discussion: https://postgr.es/m/20260707185751.f9.noahmisch@microsoft.com 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.
-
Re: SPLIT/MERGE use of is_internal=true
Alexander Korotkov <aekorotkov@gmail.com> — 2026-07-08T00:18:18Z
On Wed, Jul 8, 2026 at 2:58 AM Noah Misch <noah@leadboat.com> 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 <noah@leadboat.com> 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 <noah@leadboat.com> > > Discussion: https://postgr.es/m/20260707185751.f9.noahmisch@microsoft.com > > 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