> On Mar 10, 2026, at 23:32, Greg Sabino Mullane <[email protected]> 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 is not 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/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?
Are you concerning that rendering the full message text is the extra work? This
is not a hot path, so I don’t think that would 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 of my 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/
v7-0001-ALTER-TABLE-emit-NOTICE-when-ONLY-is-omitted-but-.patch
Description: Binary data
