Re: Testing autovacuum wraparound (including failsafe) - Mailing list pgsql-hackers

On Thu, 28 Sept 2023 at 03:55, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Sorry for the late reply.
>
> On Sun, Sep 3, 2023 at 2:48 PM Noah Misch <noah@leadboat.com> wrote:
> >
> > On Wed, Jul 12, 2023 at 01:47:51PM +0200, Daniel Gustafsson wrote:
> > > > On 12 Jul 2023, at 09:52, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > Agreed. The timeout can be set by manually setting
> > > > PG_TEST_TIMEOUT_DEFAULT, but I bump it to 10 min by default. And it
> > > > now require setting PG_TET_EXTRA to run it.
> > >
> > > +# bump the query timeout to avoid false negatives on slow test syetems.
> > > typo: s/syetems/systems/
> > >
> > >
> > > +# bump the query timeout to avoid false negatives on slow test syetems.
> > > +$ENV{PG_TEST_TIMEOUT_DEFAULT} = 600;
> > > Does this actually work?  Utils.pm read the environment variable at compile
> > > time in the BEGIN block so this setting won't be seen?  A quick testprogram
> > > seems to confirm this but I might be missing something.
> >
> > The correct way to get a longer timeout is "IPC::Run::timer(4 *
> > $PostgreSQL::Test::Utils::timeout_default);".  Even if changing env worked,
> > that would be removing the ability for even-slower systems to set timeouts
> > greater than 10min.
>
> Agreed.
>
> I've attached new version patches. 0001 patch adds an option to
> background_psql to specify the timeout seconds, and 0002 patch is the
> main regression test patch.

Few comments:
1) Should we have some validation for the inputs given:
+PG_FUNCTION_INFO_V1(consume_xids_until);
+Datum
+consume_xids_until(PG_FUNCTION_ARGS)
+{
+       FullTransactionId targetxid =
FullTransactionIdFromU64((uint64) PG_GETARG_INT64(0));
+       FullTransactionId lastxid;
+
+       if (!FullTransactionIdIsNormal(targetxid))
+               elog(ERROR, "targetxid %llu is not normal", (unsigned
long long) U64FromFullTransactionId(targetxid));

If not it will take inputs like -1 and 999999999999999.
Also the notice messages might confuse for the above values, as it
will show a different untilxid value like the below:
postgres=# SELECT consume_xids_until(999999999999999);
NOTICE:  consumed up to 0:10000809 / 232830:2764472319

2) Should this be added after worker_spi as we generally add it in the
alphabetical order:
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index fcd643f6f1..4054bde84c 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -10,6 +10,7 @@ subdir('libpq_pipeline')
 subdir('plsample')
 subdir('spgist_name_ops')
 subdir('ssl_passphrase_callback')
+subdir('xid_wraparound')
 subdir('test_bloomfilter')

3) Similarly here too:
index e81873cb5a..a4c845ab4a 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -13,6 +13,7 @@ SUBDIRS = \
                  libpq_pipeline \
                  plsample \
                  spgist_name_ops \
+                 xid_wraparound \
                  test_bloomfilter \

4) The following includes are not required transam.h, fmgr.h, lwlock.h
+ *             src/test/modules/xid_wraparound/xid_wraparound.c
+ *
+ * -------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "access/transam.h"
+#include "access/xact.h"
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "storage/lwlock.h"
+#include "storage/proc.h"

Regards,
Vignesh