On Tuesday, January 24, 2023 3:19 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for v86-0002
>
> ======
> Commit message
>
> 1.
> Use the use the existing developer option logical_replication_mode to test the
> parallel apply of large transaction on subscriber.
>
> ~
>
> Typo “Use the use the”
>
> SUGGESTION (rewritten)
> Give additional functionality to the existing developer option
> 'logical_replication_mode' to help test parallel apply of large transactions on the
> subscriber.
Changed.
> ~~~
>
> 2.
> Maybe that commit message should also say extra TAP tests that have been
> added to exercise the serialization part of the parallel apply?
Added.
> BTW – I can see the TAP tests are testing full serialization (when the GUC is
> 'immediate') but I not sure how is "partial" serialization (when it has to switch
> halfway from shmem to files) being tested.
The new tests are intended to test most of new code patch for partial
serialization by doing it from the beginning. Later, if required, we can add
different tests for it.
>
> ======
> doc/src/sgml/config.sgml
>
> 3.
> Allows streaming or serializing changes immediately in logical decoding. The
> allowed values of logical_replication_mode are buffered and immediate. When
> set to immediate, stream each change if streaming option (see optional
> parameters set by CREATE SUBSCRIPTION) is enabled, otherwise, serialize each
> change. When set to buffered, which is the default, decoding will stream or
> serialize changes when logical_decoding_work_mem is reached.
> On the subscriber side, if streaming option is set to parallel, this parameter also
> allows the leader apply worker to send changes to the shared memory queue or
> to serialize changes. When set to buffered, the leader sends changes to parallel
> apply workers via shared memory queue. When set to immediate, the leader
> serializes all changes to files and notifies the parallel apply workers to read and
> apply them at the end of the transaction.
>
> ~
>
> Because now this same developer GUC affects both the publisher side and the
> subscriber side differently IMO this whole description should be re-structured
> accordingly.
>
> SUGGESTION (something like)
>
> The allowed values of logical_replication_mode are buffered and immediate. The
> default is buffered.
>
> On the publisher side, ...
>
> On the subscriber side, ...
Changed.
>
> ~~~
>
> 4.
> This parameter is intended to be used to test logical decoding and replication of
> large transactions for which otherwise we need to generate the changes till
> logical_decoding_work_mem is reached.
>
> ~
>
> Maybe this paragraph needs rewording or moving. e.g. Isn't that misleading
> now? Although this might be an explanation for the publisher side, it does not
> seem relevant to the subscriber side's behaviour.
Adjusted the description here.
>
> ======
> .../replication/logical/applyparallelworker.c
>
> 5.
> @ -1149,6 +1149,9 @@ pa_send_data(ParallelApplyWorkerInfo *winfo, Size
> nbytes, const void *data)
> Assert(!IsTransactionState());
> Assert(!winfo->serialize_changes);
>
> + if (logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE) return
> + false;
> +
>
> I felt that code should have some comment, even if it is just something quite
> basic like "/* For developer testing */"
Added.
>
> ======
> .../t/018_stream_subxact_abort.pl
>
> 6.
> +# Clean up test data from the environment.
> +$node_publisher->safe_psql('postgres', "TRUNCATE TABLE test_tab_2");
> +$node_publisher->wait_for_catchup($appname);
>
> Is it necessary to TRUNCATE the table here? If everything is working shouldn't
> the data be rolled back anyway?
I think it's unnecessary, so removed.
>
> ~~~
>
> 7.
> +$node_publisher->safe_psql(
> + 'postgres', q{
> + BEGIN;
> + INSERT INTO test_tab_2 values(1);
> + SAVEPOINT sp;
> + INSERT INTO test_tab_2 values(1);
> + ROLLBACK TO sp;
> + COMMIT;
> + });
>
> Perhaps this should insert 2 different values so then the verification code can
> check the correct value remains instead of just checking COUNT(*)?
I think testing the count should be ok as the nearby testcases are
also checking the count.
Best regards,
Hou zj