public inbox for [email protected]  

help / color / mirror / Atom feed

From: Greg Sabino Mullane <[email protected]>
To: Jim Jones <[email protected]>
Cc: Chao Li <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: ALTER TABLE: warn when actions do not recurse to partitions
Date: Tue, 10 Mar 2026 11:32:29 -0400
Message-ID: <CAKAnmmKzzOWKQVhaAt8LrBmUW=FLWBqS4v8cwKOG53hBToQZRA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAEoWx2=SLga-xH09Cq_PAvsHhQHrBK+V0vF821JKgzS=Bm0haA@mail.gmail.com>
	<CAKFQuwZXt2dAuHFs6MtRCzfMP6YZFzssuzdW2woJBVmco=CDdQ@mail.gmail.com>
	<CAEoWx2nic2MaPUEKwb0Me1iwO3LsCf8wUHm55pjqaTRgJEZpcQ@mail.gmail.com>
	<CAKFQuwahxpPKT-LXGJ34BO5cVBUxFxzgNPTY8E_VW-cuNU_DkQ@mail.gmail.com>
	<CAEoWx2kQ9aSn-0LGPKc=woj+h-HR4uBV9TL20aeM3HzLmFqgFQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CAEoWx2mZsWnyS_XLDL5mL+yuU_g65p_tmHNesyfngqow7gtS3A@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Review of v6:

typedef struct partitionNoRecurseNotice
> {
>        List       *notices;
> }                      partitionNoRecurseNotice;


Not sure why we need a struct here, rather than just passing the list
around?

Also should be PartitionNoRecurseNotice (CamelCase)

               foreach(cell, postNotice->notices)
>                {
>                        if (strcmp((char *) lfirst(cell), notice_msg) == 0)
>                        {
>                                pfree(notice_msg);
>                                found = true;
>                                break;
>                        }
>                }


This seems a lot of extra work that could be avoided. Since we know each
message is unique to the cmdtype/AlterTableType and the rel/Relation
combination, use those two to drive the duplicate check. Then we can only
build the notice_msg if needed! Perhaps adding two more fields to that
lonely struct above?

 partitionNoRecurseNotice * postNotice);


postNotice is a little misleading - maybe pending_notices or just notices?

does not affect present partitions


s/present/existing/g


>   CollectPartitionNoRecurseNotice(AT_SetSchema, rel, stmt->relation->inh,
> false, &postNotice);


This hard-coded AT_SetSchema just to return a "SET SCHEMA" later on feels
hacky. Don't have a workaround off the top of my head, just registering my
mild unease. :)

               /* Emit a notice only if there are partitions */
>                if (nparts == 0)
>                        return;


It doesn't look like this particular case is tested. Other than that, the
tests look very good.


Cheers,
Greg

view thread (19+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: ALTER TABLE: warn when actions do not recurse to partitions
  In-Reply-To: <CAKAnmmKzzOWKQVhaAt8LrBmUW=FLWBqS4v8cwKOG53hBToQZRA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox