Re: logical changeset generation v5

On Fri, Aug 30, 2013 at 11:19 AM, Andres Freund <andres(at)2ndquadrant(dot)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.

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

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.

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

> 0006 wal_decoding: copydir: move fsync_fname to fd.[c.h] and make it public
> * Pretty trivial and boring.

Seems fine.

> 0007 wal_decoding: Add information about a tables primary key to struct RelationData
> * Could be used in the matview refresh code

I think you and Kevin should discuss whether this is actually the
right way to do this. ISTM that if logical replication and
materialized views end up selecting different approaches to this
problem, everybody loses.

> 0002 wal_decoding: Introduce InvalidCommandId and declare that to be the new maximum for CommandCounterIncrement

I'm still unconvinced we want this.

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