Dear Horiguchi-san, Thank you for checking the patch! PSA new version.
> 0002: > > This patch doesn't seem to offer a means to change the default > walsender behavior. We need a subscription option named like > "walsender_exit_mode" to do that. As I said in another mail[1], I'm thinking the feature does not have to be used alone for now. > +ConsumeWalsenderOptions(List *options, WalSndData *data) > > I wonder if it is the right design to put options for different things > into a single list. I rather choose to embed the walsender option in > the syntax than needing this function. > > K_START_REPLICATION opt_slot opt_physical RECPTR opt_timeline > opt_shutdown_mode > > K_START_REPLICATION K_SLOTIDENT K_LOGICAL RECPTR > opt_shutdown_mode plugin_options > > where opt_shutdown_mode would be like "SHUTDOWN_MODE immediate". Right, the option handling was quite bad. I added new syntax opt_shutdown_mode to logical replication. And many codes were modified accordingly. Note that based on the other discussion, I removed codes for supporting physical replication but tried to keep the extensibility. > ====== > If we go with the current design, I think it is better to share the > option list rule between the logical and physical START_REPLCIATION > commands. > > I'm not sure I like the option syntax > "exit_before_confirming=<Boolean>". I imagin that other options may > come in future. Thus, how about "walsender_shutdown_mode=<mode>", > where the mode is one of "wait_flush"(default) and "immediate"? Seems better, I changed to from boolean to enumeration. > +typedef struct > +{ > + bool exit_before_confirming; > +} WalSndData; > > Data doesn't seem to represent the variable. Why not WalSndOptions? This is inspired by PGOutputData, but I prefer your idea. Fixed. > - !equal(newsub->publications, MySubscription->publications)) > + !equal(newsub->publications, MySubscription->publications) || > + (newsub->minapplydelay > 0 && > MySubscription->minapplydelay == 0) || > + (newsub->minapplydelay == 0 && > MySubscription->minapplydelay > 0)) > > I slightly prefer the following expression (Others may disagree:p): > > ((newsub->minapplydelay == 0) != (MySubscription->minapplydelay == 0)) I think conditions for the same parameter should be aligned one line, So your posted seems better. Fixed. > > And I think we need a comment for the term. For example, > > /* minapplydelay affects START_REPLICATION option exit_before_confirming > */ Added just above the condition. > + * Reads all entrly of the list and consume if needed. > s/entrly/entries/ ? > ... This part is no longer needed. [1]: https://www.postgresql.org/message-id/TYAPR01MB5866D3EC780D251953BDE7FAF5D89%40TYAPR01MB5866.jpnprd01.prod.outlook.com Best Regards, Hayato Kuroda FUJITSU LIMITED
v4-0001-Time-delayed-logical-replication-subscriber.patch
Description: v4-0001-Time-delayed-logical-replication-subscriber.patch
v4-0002-Extend-START_REPLICATION-command-to-accept-walsen.patch
Description: v4-0002-Extend-START_REPLICATION-command-to-accept-walsen.patch