Re: logical changeset generation v5 - Mailing list pgsql-hackers
From | Robert Haas |
---|---|
Subject | Re: logical changeset generation v5 |
Date | |
Msg-id | CA+Tgmob72r8PTRVNr43FKDbfqyoq6aujg9Wd+F8EVd-S_D=gJQ@mail.gmail.com Whole thread Raw |
In response to | Re: logical changeset generation v5 (Andres Freund <andres@2ndquadrant.com>) |
Responses |
Re: logical changeset generation v5
Re: lcr v5 - introduction of InvalidCommandId Re: logical changeset generation v5 Re: lcr v5 - primary/candidate key in relcache |
List | pgsql-hackers |
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. 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
pgsql-hackers by date: