Hi,

I did look at the code a bit. The first 6 patches seem reasonable.
I don't understand why some patches are separate tbh (like 7-10, or 11).

About the 0009:
> diff --git a/contrib/pg_visibility/pg_visibility.c 
> b/contrib/pg_visibility/pg_visibility.c
> index 9985e3e..4fa3ad4 100644
> --- a/contrib/pg_visibility/pg_visibility.c
> +++ b/contrib/pg_visibility/pg_visibility.c
> @@ -538,7 +538,7 @@ collect_corrupt_items(Oid relid, bool all_visible, bool 
> all_frozen)
>       if (all_visible)
>       {
>               /* Don't pass rel; that will fail in recovery. */
> -             OldestXmin = GetOldestXmin(NULL, true);
> +             OldestXmin = GetOldestXmin(NULL, true, false);
>       }
>  
>       rel = relation_open(relid, AccessShareLock);
> @@ -660,7 +660,7 @@ collect_corrupt_items(Oid relid, bool all_visible, bool 
> all_frozen)
>                                * a buffer lock. And this shouldn't happen 
> often, so it's
>                                * worth being careful so as to avoid false 
> positives.
>                                */
> -                             RecomputedOldestXmin = GetOldestXmin(NULL, 
> true);
> +                             RecomputedOldestXmin = GetOldestXmin(NULL, 
> true, false);
>  
>                               if (!TransactionIdPrecedes(OldestXmin, 
> RecomputedOldestXmin))
>                                       record_corrupt_item(items, 
> &tuple.t_self);
> diff --git a/contrib/pgstattuple/pgstatapprox.c 
> b/contrib/pgstattuple/pgstatapprox.c
> index f524fc4..5b33c97 100644
> --- a/contrib/pgstattuple/pgstatapprox.c
> +++ b/contrib/pgstattuple/pgstatapprox.c
> @@ -70,7 +70,7 @@ statapprox_heap(Relation rel, output_type *stat)
>       TransactionId OldestXmin;
>       uint64          misc_count = 0;
>  
> -     OldestXmin = GetOldestXmin(rel, true);
> +     OldestXmin = GetOldestXmin(rel, true, false);
>       bstrategy = GetAccessStrategy(BAS_BULKREAD);
>  
>       nblocks = RelationGetNumberOfBlocks(rel);

This does not seem correct, you are sending false as pointer parameter.

0012:

I think there should be parameter saying if snapshot should be exported
or not and if user asks for it on standby it should fail.

0014 makes 0011 even more pointless.

Not going into deeper detail as this is still very WIP. I go agree with
the general design though.

This also replaces the previous timeline following and decoding
threads/CF entries so maybe those should be closed in CF?

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to