Re: logical changeset generation v5 - Mailing list pgsql-hackers

On 2013-09-03 11:40:57 -0400, Robert Haas wrote:
> On Fri, Aug 30, 2013 at 11:19 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > 0005 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done
> > * benefits hot standby startup
> 
> Review:
> 
> 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?

> 2. I think the variable naming could be better.  If nothing else, I'd
> spell out "snapshot" rather than abbreviating it to "snap".  I'd also
> add comments explaining what each of those variables does.

Ok.

> And why
> isn't log_snap_interval_ms a #define rather than a variable?  (Don't
> even talk to me about using gdb on a running instance.  If you're even
> thinking about that, this needs to be a GUC.)

Ugh. It certainly doesn't have anything to do with wanting to change it
on a running system using gdb. Brrr.

I think I wanted it to be a constant variable but forgot the const. I
personally prefer 'static const' to #define's if its legal C, but I
guess the project's style differs, so I'll change that.

> 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.

> > 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.

>  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.

Greetings,

Andres Freund

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