Re: Improving connection scalability: GetSnapshotData()

On 2020-07-24 18:15:15 -0300, Ranier Vilela wrote:
> Em sex., 24 de jul. de 2020 às 14:16, Andres Freund <andres@anarazel.de>
> escreveu:
>
> > On 2020-07-24 14:05:04 -0300, Ranier Vilela wrote:
> > > Latest Postgres
> > > Windows 64 bits
> > > msvc 2019 64 bits
> > >
> > > Patches applied v12-0001 to v12-0007:
> > >
> > >  C:\dll\postgres\contrib\pgstattuple\pgstatapprox.c(74,28): warning
> > C4013:
> > > 'GetOldestXmin' indefinido; assumindo extern retornando int
> > > [C:\dll\postgres
> > > C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(569,29): warning
> > > C4013: 'GetOldestXmin' indefinido; assumindo extern retornando int
> > > [C:\dll\postgres\pg_visibility.
> > > vcxproj]
> > >  C:\dll\postgres\contrib\pgstattuple\pgstatapprox.c(74,56): error C2065:
> > > 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado
> > > [C:\dll\postgres\pgstattuple.vcxproj]
> > >   C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(569,58): error
> > > C2065: 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado
> > > [C:\dll\postgres\pg_visibility.vcxproj]
> > >   C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(686,70): error
> > > C2065: 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado
> > > [C:\dll\postgres\pg_visibility.vcxproj]
> >
> > I don't know that's about - there's no call to GetOldestXmin() in
> > pgstatapprox and pg_visibility after patch 0002? And similarly, the
> > PROCARRAY_* references are also removed in the same patch?
> >
> Maybe need to remove them from these places, not?
> C:\dll\postgres\contrib>grep -d GetOldestXmin *.c
> File pgstattuple\pgstatapprox.c:
>         OldestXmin = GetOldestXmin(rel, PROCARRAY_FLAGS_VACUUM);
> File pg_visibility\pg_visibility.c:
>                 OldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM);
>                                  * deadlocks, because surely
> GetOldestXmin() should never take
>                                 RecomputedOldestXmin = GetOldestXmin(NULL,
> PROCARRAY_FLAGS_VACUUM);

The 0002 patch changed those files:

diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 68d580ed1e0..37206c50a21 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -563,17 +563,14 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
        BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD);
        TransactionId OldestXmin = InvalidTransactionId;

-       if (all_visible)
-       {
-               /* Don't pass rel; that will fail in recovery. */
-               OldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM);
-       }
-
        rel = relation_open(relid, AccessShareLock);

        /* Only some relkinds have a visibility map */
        check_relation_relkind(rel);

+       if (all_visible)
+               OldestXmin = GetOldestNonRemovableTransactionId(rel);
+
        nblocks = RelationGetNumberOfBlocks(rel);

        /*
@@ -679,11 +676,12 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
                                 * From a concurrency point of view, it sort of sucks to
                                 * retake ProcArrayLock here while we're holding the buffer
                                 * exclusively locked, but it should be safe against
-                                * deadlocks, because surely GetOldestXmin() should never take
-                                * a buffer lock. And this shouldn't happen often, so it's
-                                * worth being careful so as to avoid false positives.
+                                * deadlocks, because surely GetOldestNonRemovableTransactionId()
+                                * should never take a buffer lock. And this shouldn't happen
+                                * often, so it's worth being careful so as to avoid false
+                                * positives.
                                 */
-                               RecomputedOldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM);
+                               RecomputedOldestXmin = GetOldestNonRemovableTransactionId(rel);

                                if (!TransactionIdPrecedes(OldestXmin, RecomputedOldestXmin))
                                        record_corrupt_item(items, &tuple.t_self);

diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index dbc0fa11f61..3a99333d443 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -71,7 +71,7 @@ statapprox_heap(Relation rel, output_type *stat)
        BufferAccessStrategy bstrategy;
        TransactionId OldestXmin;

-       OldestXmin = GetOldestXmin(rel, PROCARRAY_FLAGS_VACUUM);
+       OldestXmin = GetOldestNonRemovableTransactionId(rel);
        bstrategy = GetAccessStrategy(BAS_BULKREAD);

        nblocks = RelationGetNumberOfBlocks(rel);