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);


Greetings,

Andres Freund