Re: logical changeset generation v5

On Tue, Sep 3, 2013 at 7:10 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> I don't think it particularly needs to be configurable, but I wonder
>> if we can't be a bit smarter about when we do it.  For example,
>> suppose we logged it every 15 s but only until we log a non-overflowed
>> snapshot.
>
> There's actually more benefits than just overflowed snapshots (pruning
> of the known xids machinery, exclusive lock cleanup).

I know that, but I thought the master and slave could only lose sync
on those things after a master crash and that once per checkpoint
cycle was enough for those other benefits.  Am I wrong?

> The patch as-is only writes if there has been WAL written since the last
> time it logged a running_xacts. I think it's not worth building more
> smarts than that?

Hmm, maybe.

>> Because I don't see any reason to believe that this WAL record is any
>> more important or urgent than any other WAL record that might get
>> logged.
>
> I can't follow the logic behind that statement. Just about all WAL
> records are either pretty immediately flushed afterwards or are done in
> the context of a transaction where we flush (or do a
> XLogSetAsyncXactLSN) at transaction commit.
>
> XLogBackgroundFlush() won't necessarily flush the running_xacts
> record.

OK, this was the key point I was missing.

>> It seems we need some more design there.  Perhaps entering replication
>> mode could be triggered by writing either dbname=replication or
>> replication=yes.  But then, do the replication commands simply become
>> SQL commands?  I've certainly seen hackers use them that way.  And I
>> can imagine that being a sensible approach, but this patch seems like
>> it's only covering a fairly small fraction of what really ought to be
>> a single commit.
>
> Yes. I think you're right that we need more input/design here. I've
> previously started threads about it, but nobody replied :(.
>
> The problem with using dbname=replication as a trigger for anything is
> that we actually allow databases to be created with that name. Perhaps
> that was a design mistake.

It seemed like a good idea at the time, but maybe it wasn't.  I'm not
sure where to go with it at this point; a forcible backward
compatibility break would probably screw things up for a lot of
people.

> I wondered about turning replication from a boolean into something like
> off|0, on|1, database. dbname= gets only used in the latter
> variant. That would be compatible with previous versions and would even
> support using old tools (since all of them seem to do replication=1).

I don't love that, but I don't hate it, either.  But it still doesn't
answer the following question, which I think is important: if I (or
someone else) commits this patch, how will that make things better for
users?  At the moment it's just adding a knob that doesn't do anything
for you when you twist it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company