On Thu, Jul 9, 2026 at 2:53 PM shveta malik <[email protected]> wrote: > > > > > To reduce duplicate code, I moved the schema > > qualification and eligibility checks for both cases to execution time >> and removed the corresponding processing from gram.y. > > > > Since this changes a fair amount of code, I've kept it as a separate > > patch (v19-0002) to make review easier. Depending on the feedback, we > > can optimize the implementation or move parts of it back into gram.y > > before merging it into patch 0001. > > ~~~ > > > > Nisha, > > - * Also flattens except_tables from TABLES IN SCHEMA nodes into the list so > - * that ObjectsInPublicationToOids() sees them as top-level > EXCEPT_TABLE entries. > + * The except_tables attached to TABLES IN SCHEMA nodes are left in place; > + * ObjectsInPublicationToOids() qualifies names, validates schema membership, > + * and flattens them once the schema OID is known. > > Can you please explain what does 'flatten' mean here. >
Each TABLES IN SCHEMA ... EXCEPT (...) entry stores its own nested list of EXCEPT tables. Flatten means collecting the PublicationTable from each nested list into a single top-level except_pubtables list, giving it the same structure as a FOR TABLE list that can be passed directly to OpenTableList(). I've reworded the comment. We can also remove it if it seems unnecessary here. > The flatten logic here is added by your patch001 only, it is not an > existing logic. To me (in my initial round of review), it seems okay > if we moved it to ObjectsInPublicationToOids(). The only thing is that > the one error (table in EXCEPT clause does not belong to schema) which > is possible to get at the parsing stage will now be emitted during > pub-creation stage. Yes, the logic was introduced in patch 0001 itself. I just highlighted it for the case you mentioned, where the error is deferred instead of being raised at parse time. > But since it will keep the entire logic of > except-list verification at one place and considering that > creat/alter-pub is not so frequent operation, I am fine with it. But > lets see if others have any comment here. > My thought was the same: to keep the logic in one place for now. Let's see if others have any opinions on it as well. > ~~ > > I had a quick look at the changes: > > + * Include the ONLY-ness of this entry in its signature. EXCEPT > + * (TABLE parent) excludes parent and its inheritance children, > + * while EXCEPT (TABLE ONLY parent) excludes only parent itself. > > I think this will unconditionally apply to paritioned tables as well > but for paritioned table, EXCEPT doc says that ONLY and * has no > effect, but here it shows the effect: > > Thsi gives error: > postgres=# CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA public EXCEPT > (TABLE ONLY tab_root), TABLES IN SCHEMA public EXCEPT (TABLE > tab_root); > ERROR: 42P17: schema "public" specified with conflicting EXCEPT lists > LOCATION: ProcessSchemaExceptTables, publicationcmds.c:299 > > While this does not: > postgres=# CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA public EXCEPT > (TABLE tab_root), TABLES IN SCHEMA public EXCEPT (TABLE tab_root); > CREATE PUBLICATION > > But I also checked column-list errors and found that even if columns > are exact sames, we get error: > > postgres=# CREATE PUBLICATION PUB4 FOR TABLE T1(i), T1(i); > ERROR: 42710: conflicting or redundant column lists for table "t1" > LOCATION: OpenTableList, publicationcmds.c:2312 > > While this works: > postgres=# CREATE PUBLICATION PUB4 FOR TABLE T1, T1; > CREATE PUBLICATION > > This makes me think that even introducing the smartness of identifying > ONLY in case of inherited tables and distinguishing it from > parititoned tables is not required. We can just change your error > message to 'conflicting or redundant EXCEPT lists for schmea "s1"' and > throw error in all cases where schmea with except list repeats without > actually comparing them. Does this sound reasonable? > Yes, that works for me as we have FOR TABLES example to follow. I've made the changes in v20. -- Thanks, Nisha
