On Tue, Nov 16, 2021 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Nov 15, 2021 at 12:38 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've updated the patch so that ProcArrayInstallRestoredXmin() sets
> > both xmin and statusFlags only when the source proc is still running
> > and xmin doesn't go backwards. IOW it doesn't happen that only one of
> > them is set by this function, which seems more understandable
> > behavior.
> >
>
> How have you tested this patch? As there was no test case presented in
> this thread, I used the below manual test to verify that the patch
> works. The idea is to generate a scenario where a parallel vacuum
> worker holds back the xmin from advancing.
>
> Setup:
> -- keep autovacuum = off in postgresql.conf
> create table t1(c1 int, c2 int);
> insert into t1 values(generate_series(1,1000),100);
> create index idx_t1_c1 on t1(c1);
> create index idx_t1_c2 on t1(c2);
>
> create table t2(c1 int, c2 int);
> insert into t2 values(generate_series(1,1000),100);
> create index idx_t2_c1 on t1(c1);
>
> Session-1:
> delete from t1 where c1 < 10; --this is to ensure that vacuum has some
> work to do
>
> Session-2:
> -- this is done just to ensure the Session-1's xmin captures the value
> of this xact
> begin;
> select txid_current(); -- say value is 725
> insert into t2 values(1001, 100);
>
> Session-1:
> set min_parallel_index_scan_size=0;
> -- attach a debugger and ensure to stop parallel worker somewhere
> before it completes and the leader after launching parallel worker
> vacuum t1;
>
> Session-2:
> -- commit the open transaction
> commit;
>
> Session-3:
> -- attach a debugger and break at the caller of vacuum_set_xid_limits.
> vacuum t2;
Yes, I've tested this patch in a similar way; while running pgbench in
the background in order to constantly consume XID, I checked if the
oldest xmin in VACUUM VERBOSE log is advancing even during parallel
vacuum running.
>
> I noticed that before the patch the value of oldestXmin in Session-3
> is 725 but after the patch it got advanced. I have made minor edits to
> the attached patch. See, if this looks okay to you then please prepare
> and test the patch for back-branches as well. If you have some other
> way to test the patch then do share the same and let me know if you
> see any flaw in the above verification method.
The patch looks good to me. But I can't come up with a stable test for
this. It seems to be hard without stopping and resuming parallel
vacuum workers. Do you have any good idea?
I've attached patches for back branches (13 and 14).
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
