Re: Logical decoding on standby

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