From: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: Perform streaming logical transactions by background workers and parallel apply |
Date: | 2022-05-13 08:48:33 |
Message-ID: | OS0PR01MB57164393B7F8A9A71C9CEB7394CA9@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thursday, May 5, 2022 1:46 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> Here are my review comments for v5-0001.
> I will take a look at the v5-0002 (TAP) patch another time.
Thanks for the comments !
> 4. Commit message
>
> User can set the streaming option to 'on/off', 'apply'. For now,
> 'apply' means the streaming will be applied via a apply background if
> available. 'on' means the streaming transaction will be spilled to
> disk.
>
>
> I think "apply" might not be the best choice of values for this
> meaning, but I think Hou-san already said [1] that this was being
> reconsidered.
Yes, I am thinking over this along with some other related stuff[1] posted by Amit
and sawada. Will change this in next version.
> 7. src/backend/commands/subscriptioncmds.c - defGetStreamingMode
>
> +static char
> +defGetStreamingMode(DefElem *def)
> +{
> + /*
> + * If no parameter given, assume "true" is meant.
> + */
> + if (def->arg == NULL)
> + return SUBSTREAM_ON;
>
> But is that right? IIUC all the docs said that the default is OFF.
I think it's right. "arg == NULL" means user specify the streaming option
without the value. Like CREATE SUBSCRIPTION xxx WITH(streaming). The value should
be 'on' in this case.
> 12. src/backend/replication/logical/origin.c - replorigin_session_setup
>
> @@ -1110,7 +1110,11 @@ replorigin_session_setup(RepOriginId node)
> if (curstate->roident != node)
> continue;
>
> - else if (curstate->acquired_by != 0)
> + /*
> + * We allow the apply worker to get the slot which is acquired by its
> + * leader process.
> + */
> + else if (curstate->acquired_by != 0 && acquire)
>
> I still feel this is overly-cofusing. Shouldn't comment say "Allow the
> apply bgworker to get the slot...".
>
> Also the parameter name 'acquire' is hard to reconcile with the
> comment. E.g. I feel all this would be easier to understand if the
> param was was refactored with a name like 'bgworker' and the code was
> changed to:
> else if (curstate->acquired_by != 0 && !bgworker)
>
> Of course, the value true/false would need to be flipped on calls too.
> This is the same as my previous comment [PSv4] #26.
I feel it's not good idea to mention bgworker in origin.c. I have remove this
comment and add some other comments in worker.c.
> 26. src/backend/replication/logical/worker.c - apply_handle_stream_abort
>
> + if (found)
> + {
> + elog(LOG, "rolled back to savepoint %s", spname);
> + RollbackToSavepoint(spname);
> + CommitTransactionCommand();
> + subxactlist = list_truncate(subxactlist, i + 1);
> + }
>
> Should that elog use the "[Apply BGW #%u]" format like the others for BGW?
I feel the "[Apply BGW #%u]" is a bit hacky and some of them comes from the old
patchset. I will recheck these logs and adjust them and change some log
level in next version.
> 27. src/backend/replication/logical/worker.c - apply_handle_stream_abort
>
> Should this function be setting stream_apply_worker = NULL somewhere
> when all is done?
> 29. src/backend/replication/logical/worker.c - apply_handle_stream_commit
>
> I am unsure, but should something be setting the stream_apply_worker =
> NULL somewhere when all is done?
I think the worker already be set to NULL in apply_handle_stream_stop.
> 32. src/backend/replication/logical/worker.c - ApplyBgwShutdown
>
> +/*
> + * Set the failed flag so that the main apply worker can realize we have
> + * shutdown.
> + */
> +static void
> +ApplyBgwShutdown(int code, Datum arg)
>
> If the 'code' param is deliberately unused it might be better to say
> so in the comment...
Not sure about this. After searching the codes, I think most of the callback
functions doesn't use and add comments for the 'code' param.
> 45. src/backend/utils/activity/wait_event.c
>
> @@ -388,6 +388,9 @@ pgstat_get_wait_ipc(WaitEventIPC w)
> case WAIT_EVENT_HASH_GROW_BUCKETS_REINSERT:
> event_name = "HashGrowBucketsReinsert";
> break;
> + case WAIT_EVENT_LOGICAL_APPLY_WORKER_READY:
> + event_name = "LogicalApplyWorkerReady";
> + break;
>
> I am not sure this is the best name for this event since the only
> place it is used (in apply_bgworker_wait_for) is not only waiting for
> READY state. Maybe a name like WAIT_EVENT_LOGICAL_APPLY_BGWORKER or
> WAIT_EVENT_LOGICAL_APPLY_WORKER_SYNC would be more appropriate? Need
> to change the wait_event.h also.
I noticed a similar named "WAIT_EVENT_LOGICAL_SYNC_STATE_CHANGE", so I changed
this to WAIT_EVENT_LOGICAL_APPLY_WORKER_STATE_CHANGE.
> 47. src/test/regress/expected/subscription.out - missting test
>
> Missing some test cases for all new option values? E.g. Where is the
> test using streaming value is set to 'apply'. Same comment as [PSv4]
> #81
The new option is tested in the second patch posted by Shi yu.
I addressed other comments from Peter and the 2PC related comment from Shi.
Here is the version patch.
Best regards,
Hou zj
Attachment | Content-Type | Size |
---|---|---|
v6-0001-Perform-streaming-logical-transactions-by-background.patch | application/octet-stream | 74.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | houzj.fnst@fujitsu.com | 2022-05-13 08:52:32 | RE: Perform streaming logical transactions by background workers and parallel apply |
Previous Message | Alvaro Herrera | 2022-05-13 08:30:12 | list of TransactionIds |