Re: logical changeset generation v5

On Tue, Sep 3, 2013 at 12:05 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> 1. I think more comments are needed here to explain why we need this.
>> I don't know if the comments should go into the functions modified by
>> this patch or in some other location, but I don't find what's here now
>> adequate for understanding.
>
> Hm. What information are you actually missing? I guess the
> XLogSetAsyncXactLSN() needs a bit more context based on your question,
> what else?
> Not sure if it makes sense to explain in detail why it helps us to get
> into a consistent state faster?

Well, we must have had some idea in mind when the original Hot Standby
patch went in that doing this once per checkpoint was good enough.
Now we think we need it every 15 seconds, but not more or less often.
So, why the change of heart?  To my way of thinking, it seems as
though we ought to always begin replay at a checkpoint, so the standby
ought always to see one of these records immediately.  Obviously
that's not good enough, but why not?  And why is every 15 seconds good
enough?

>> 3. Why does LogCurrentRunningXacts() need to call
>> XLogSetAsyncXactLSN()?  Hopefully any WAL record is going to get
>> sync'd in a reasonably timely fashion; I can't see off-hand why this
>> one should need special handling.
>
> No, we don't force writing out wal records in a timely fashion if
> there's no pressure in wal_buffers, basically only on commits and
> various XLogFlush()es. It doesn't make much of a difference if the
> entire system is busy, but if it's not the wal writer will sleep. The
> alternative would be to XLogFlush() the record, but that would actually
> block, which isn't really what we want/need.

The WAL writer is supposed to call XLogBackgroundFlush() every time
WalWriterDelay expires.  Yeah, it can hibernate, but if it's
hibernating, then we should respect that decision for this WAL record
type also.

>> > 0003 wal_decoding: Allow walsender's to connect to a specific database
>> > * biggest problem is how to specify the connection we connect
>> >   to. Currently with the patch walsender connects to a database if it's
>> >   not named "replication" (via dbname). Perhaps it's better to invent a
>> >   replication_dbname parameter?
>
>> I understand why logical replication needs to connect to a database,
>> but I don't understand why any other walsender would need to connect
>> to a database.
>
> Well, logical replication actually streams out data using the walsender,
> so that's the reason why I want to add it there. But there have been
> cases in the past where we wanted to do stuff in the walsender that need
> database access, but we couldn't do so because you cannot connect to
> one.

Could you be more specific?

>>  Absent a clear use case for such a thing, I don't
>> think we should allow it.  Ignorant suggestion: perhaps the database
>> name could be stored in the logical replication slot.
>
> The problem is that you need to InitPostgres() with a database. You
> cannot do that again, after connecting with an empty database which we
> do in a plain walsender.

Are you saying that the logical replication slot can't be read before
calling InitPostgres()?

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