public inbox for [email protected]
help / color / mirror / Atom feedFrom: [email protected] <[email protected]>
To: vignesh C <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: RE: Skipping schema changes in publication
Date: Thu, 14 Apr 2022 09:33:15 +0000
Message-ID: <OS3PR01MB6275C6E3901B8C918B0CF0AB9EEF9@OS3PR01MB6275.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CALDaNm3Rc2KxGzLy-4qxFS+MWS6GN-ROVoPEb1MFA53W3iAqog@mail.gmail.com>
References: <CALDaNm3=JrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh=tamA@mail.gmail.com>
<CALDaNm1G0=0J2O+6pMStKuWUjxJJT5vbO-5jiUqzYVZM_sJ2+w@mail.gmail.com>
<CALDaNm3Rc2KxGzLy-4qxFS+MWS6GN-ROVoPEb1MFA53W3iAqog@mail.gmail.com>
On Tue, Apr 12, 2022 at 2:23 PM vignesh C <[email protected]> wrote:
> The patch does not apply on top of HEAD because of the recent commit,
> attached patch is rebased on top of HEAD.
Thanks for your patches.
Here are some comments for v1-0001:
1.
I found the patch add the following two new functions in gram.y:
preprocess_alltables_pubobj_list, check_skip_in_pubobj_list.
These two functions look similar. So could we just add one new function?
Besides, do we need the API `location` in new function
preprocess_alltables_pubobj_list? It seems that "location" is not used in this
new function.
In addition, the location of error cursor in the messages seems has a little
problem. For example:
postgres=# create publication pub for all TABLES skip all tables in schema public, table test;
ERROR: only SKIP ALL TABLES IN SCHEMA or SKIP TABLE can be specified with ALL TABLES option
LINE 1: create publication pub for all TABLES skip all tables in sch...
^
(The location of error cursor is under 'create')
2. I think maybe there is a minor missing in function
preprocess_alltables_pubobj_list and check_skip_in_pubobj_list:
We seem to be missing the CURRENT_SCHEMA case.
For example(In function preprocess_alltables_pubobj_list) :
+ /* Only SKIP ALL TABLES IN SCHEMA option supported with ALL TABLES */
+ if (pubobj->pubobjtype != PUBLICATIONOBJ_TABLES_IN_SCHEMA ||
+ !pubobj->skip)
maybe need to be changed like this:
+ /* Only SKIP ALL TABLES IN SCHEMA option supported with ALL TABLES */
+ if ((pubobj->pubobjtype != PUBLICATIONOBJ_TABLES_IN_SCHEMA &&
+ pubobj->pubobjtype != PUBLICATIONOBJ_TABLES_IN_CUR_SCHEMA) &&
+ pubobj->skip)
3. I think maybe there are some minor missing in create_publication.sgml.
+ [ FOR ALL TABLES [SKIP ALL TABLES IN SCHEMA { <replaceable class="parameter">schema_name</replaceable> | CURRENT_SCHEMA }]
maybe need to be changed to this:
+ [ FOR ALL TABLES [SKIP ALL TABLES IN SCHEMA { <replaceable class="parameter">schema_name</replaceable> | CURRENT_SCHEMA } [, ... ]]
4. The error message of function CreatePublication.
Does the message below need to be modified like the comment?
In addition, I think maybe "FOR/SKIP" is better.
@@ -835,18 +843,21 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
- /* FOR ALL TABLES IN SCHEMA requires superuser */
+ /* FOR [SKIP] ALL TABLES IN SCHEMA requires superuser */
if (list_length(schemaidlist) > 0 && !superuser())
ereport(ERROR,
errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to create FOR ALL TABLES IN SCHEMA publication"));
5.
I think there are some minor missing in tab-complete.c.
+ Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "SKIP", "ALL", "TABLES", "IN", "SCHEMA"))
maybe need to be changed to this:
+ Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL", "TABLES", "SKIP", "ALL", "TABLES", "IN", "SCHEMA"))
+ Matches("CREATE", "PUBLICATION", MatchAny, "SKIP", "FOR", "ALL", "TABLES", "IN", "SCHEMA", MatchAny)) &&
maybe need to be changed to this:
+ Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL", "TABLES", "SKIP", "ALL", "TABLES", "IN", "SCHEMA", MatchAny)) &&
6.
In function get_rel_sync_entry, do we need `if (!publish)` in below code?
I think `publish` is always false here, as we delete the check for
"pub->alltables".
```
- /*
- * If this is a FOR ALL TABLES publication, pick the partition root
- * and set the ancestor level accordingly.
- */
- if (pub->alltables)
- {
- ......
- }
-
if (!publish)
```
Regards,
Wang wei
view thread (368+ 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]
Subject: RE: Skipping schema changes in publication
In-Reply-To: <OS3PR01MB6275C6E3901B8C918B0CF0AB9EEF9@OS3PR01MB6275.jpnprd01.prod.outlook.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