Re: logical changeset generation v6.6

Hi,

On 2013-11-12 12:13:54 -0500, Robert Haas wrote:
> On Mon, Nov 11, 2013 at 12:00 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > [ updated patch-set ]
>
> I'm pretty happy with what's now patch #1, f/k/a known as patch #3,
> and probably somewhere else in the set before that. At any rate, I
> refer to 0001-wal_decoding-Add-wal_level-logical-and-log-data-requ.patch.gz.

Cool.

> I think the documentation still needs a bit of work. It's not
> particularly clean to just change all the places that refer to the
> need to set wal_level to archive (or hot_standby) level to instead
> refer to archive (or hot_standby, logical). If we're going to do it
> that way, I think we definitely need a connecting word between
> hot_standby and logical, specifically "or".

Hm. I tried to make it "archive, hot_standby or logical", but I see I
screwed up along the way.

> But I'm wondering if
> would be better to instead change those places to say archive (or any
> higher setting).

Works for me. We'd need to make sure there's a clear ordering
recognizable in at least one place, but that's a good idea anyway.

> You've actually changed the meaning of this section (and not in a good way):
>
> be set at server start. <varname>wal_level</> must be set
> - to <literal>archive</> or <literal>hot_standby</> to allow
> - connections from standby servers.
> + to <literal>archive</>, <literal>hot_standby</> or <literal>logical</>
> + to allow connections from standby servers.
>
> I think that the previous text meant that you needed archive - or, if
> you want to allow connections, hot_standby. The new text loses that
> nuance.

Yea, that's because it was lost on me in the first place...

> I'm tempted to think that we're better off calling this "logical
> decoding" rather than "logical replication". At least, we should
> standardize on one or the other. If we go with "decoding", then fix
> these:

I agree. It all used to be "logical replication" but this feature really
isn't about the replication, but about the extraction part.

>
> + * For logical replication, we need the tuple even if
> we're doing a
> +/* Do we need to WAL-log information required only for Hot Standby
> and logical replication? */
> +/* Do we need to WAL-log information required only for logical replication? */
> (and we should go back and correct the instance already added to the
> ALTER TABLE documentation)
>
> Is there any special reason why RelationIsLogicallyLogged(), which is
> basically a three-pronged test, does one of those tests in a macro and
> defers the other two to a function? Why not just put it all in the
> macro?

We could, I basically didn't want to add too much inlined code
everywhere when wal_level != logical, but the functions reduced in size
since.

> I did some performance testing on the previous iteration of this
> patch, just my usual set of 30-minute pgbench runs. I tried it with
> wal_level=hot_standby and wal_level=logical. 32-clients, scale factor
> 300, shared_buffers = 8GB, maintenance_work_mem = 4GB,
> synchronous_commit = off, checkpoint_segments = 300,
> checkpoint_timeout = 15min, checkpoint_completion_target = 0.9. The
> results came out like this:
>
> hot_standby tps = 15070.229005 (including connections establishing)
> hot_standby tps = 14769.905054 (including connections establishing)
> hot_standby tps = 15119.350014 (including connections establishing)
> logical tps = 14713.523689 (including connections establishing)
> logical tps = 14799.242855 (including connections establishing)
> logical tps = 14557.538038 (including connections establishing)
>
> The runs were interleaved, but I've shown them here grouped by the
> wal_level in use. If you compare the median values, there's about a
> 1% regression there with wal_level=logical, but that might not even be
> significant - and if it is, well, that's why this feature has an off
> switch.

That matches my test and is imo pretty ok. The overhead is from a slight
increase in wal volume because during FPWs we do not just log the FPW
but also the tuples.
It will be worse if primary keys were changed regularly though.

> - * than its parent. Musn't recurse here, or we might get a
> stack overflow
> + * than its parent. May not recurse here, or we might get a
> stack overflow
>
> You don't need this change; it doesn't change the meaning.

I thought that "Musn't" was a typo, because of the missing t before the
n. But it obviously doesn't have to be part of this patch.

> + * with fewer than PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine
> + * if didLogXid isn't set for a transaction even though it appears
> + * in a wal record, we'll just superfluously log something.
>
> It'd be good to rewrite this comment to explain under what
> circumstances that can happen, or why it can't happen but that it
> would be OK if it did.

Ok.

> I think we'd better separate the changes to catalog.c from the rest of
> this. Those are changing semantics in a significant way that needs to
> be separately called out. In particular, a user-created table in
> pg_catalog will be able to have indexes defined on it, will be able to
> be truncated, will be allowed to have triggers, etc. I think that's
> OK, but it shouldn't be a by-blow of the rest of this patch.

Completely agreed. As evidenced by the fact that the current change
doesn't update all relevant comments & code. I wonder if we shouldn't
leave the function the current way and just add a new function for the
new behaviour.
The hard thing with that would be coming up with a new
name. IsSystemRelationId() having a different behaviour than
IsSystemRelation() seems strange to me, so just keeping that and
adapting the callers seems wrong to me.
IsInternalRelation()? IsCatalogRelation()?

Thanks for the review,

Andres Freund

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