Hi,
On Tuesday, February 7, 2023 6:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Feb 7, 2023 at 8:22 AM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Thank you for reviewing! PSA new version.
> >
>
> Few comments:
> =============
Thanks for your comments !
> 1.
> @@ -74,6 +74,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
> BKI_SHARED_RELATION BKI_ROW
>
> Oid subowner BKI_LOOKUP(pg_authid); /* Owner of the subscription */
>
> + int32 subminapplydelay; /* Replication apply delay (ms) */
> +
> bool subenabled; /* True if the subscription is enabled (the
> * worker should be running) */
>
> @@ -120,6 +122,7 @@ typedef struct Subscription
> * in */
> XLogRecPtr skiplsn; /* All changes finished at this LSN are
> * skipped */
> + int32 minapplydelay; /* Replication apply delay (ms) */
> char *name; /* Name of the subscription */
> Oid owner; /* Oid of the subscription owner */
>
> Why the new parameter is placed at different locations in above two
> strcutures? I think it should be after owner in both cases and accordingly its
> order should be changed in GetSubscription() or any other place it is used.
Fixed.
>
> 2. A minor comment change suggestion:
> /*
> * Common spoolfile processing.
> *
> - * The commit/prepare time (finish_ts) for streamed transactions is required
> - * for time-delayed logical replication.
> + * The commit/prepare time (finish_ts) is required for time-delayed
> + logical
> + * replication.
> */
Fixed.
> 3. I find the newly added tests take about 8s on my machine which is close
> highest in the subscription folder. I understand that it can't be less than 3s
> because of the delay but checking multiple cases makes it take that long. I
> think we can avoid the tests for streaming and disable the subscription. Also,
> after removing those, I think it would be better to add the remaining test in
> 001_rep_changes to save set-up and tear-down costs as well.
Sounds good to me. Moved the test to 001_rep_changes.pl.
> 4.
> +$node_publisher->append_conf('postgresql.conf',
> + 'logical_decoding_work_mem = 64kB');
>
> I think this setting is also not required.
Yes. And, in the process to move the test, removed.
Attached the v31 patch.
Note that regarding the translator style,
I chose to export the parameters from the errmsg to outside
at this stage. If there is a need to change it, then I'll follow it.
Other changes are minor alignments to make 'if' conditions
that exceeded 80 characters folded and look nicer.
Also conducted pgindent and pgperltidy.
Best Regards,
Takamichi Osumi