> On Mar 10, 2026, at 23:32, Greg Sabino Mullane <htamfids@gmail.com> wrote:
>
> Review of v6:
Thank you very much for the review.
>
> typedef struct partitionNoRecurseNotice
> {
> List *notices;
> } partitionNoRecurseNotice;
> Not sure why we need a struct here, rather than just passing the list around?
Initially I thought there might be a few fields in the structure, but ended up only one List field. Yes, this structure
isnot needed now. Removed it in v7.
>
> 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/AlterTableTypeand the rel/Relation combination, use those two to drive the duplicate check. Then we can only
buildthe notice_msg if needed! Perhaps adding two more fields to that lonely struct above?
Are you concerning that rendering the full message text is the extra work? This is not a hot path, so I don’t think
thatwould be a big deal. Actually, adding two more fields sounds more expensive.
>
> partitionNoRecurseNotice * postNotice);
>
> postNotice is a little misleading - maybe pending_notices or just notices?
Yes, “pending” makes sense. Updated in v7.
>
> 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
ofmy head, just registering my mild unease. :)
Yes, as SET SCHEMA doesn’t go through the standard ALTER TABLE process: AlterTable() -> ATController() -> ATPrepCmd().
If you get some idea, please let me know.
>
> /* 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.
Good catch. Added a test case to cover that.
PFA v7: addressed Greg’s review comments.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
