On 19 March 2017 at 22:12, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote:
> I am slightly worried about impact of the readTimeLineHistory() call but > I think it should be called so little that it should not matter. Pretty much my thinking too. > That brings us to the big patch 0003. > > I still don't like the "New in 10.0" comments in documentation, for one > it's 10, not 10.0 and mainly we don't generally write stuff like this to > documentation, that's what release notes are for. OK. Personally I think it's worthwhile for protocol docs, which are more dev-focused. But I agree it's not consistent with the rest of the docs, so removed. (Frankly I wish we did this consistently throughout the Pg docs, too, and it'd be much more user-friendly if we did, but that's just not going to happen.) > There is large amounts of whitespace mess (empty lines with only > whitespace, spaces at the end of the lines), nothing horrible, but > should be cleaned up. Fixed. > One thing I don't understand much is the wal_level change and turning > off catalogXmin tracking. I don't really see anywhere that the > catalogXmin would be reset in control file for example. There is TODO in > UpdateOldestCatalogXmin() that seems related but tbh I don't follow > what's happening there - comment says about not doing anything, but the > code inside the if block are only Asserts. UpdateOldestCatalogXmin(...) with force=true forces a XactLogCatalogXminUpdate(...) call to write the new procArray->replication_slot_catalog_xmin . We call it with force=true from XLogReportParameters(...) when wal_level has been lowered; see XLogReportParameters. This will write out a xl_xact_catalog_xmin_advance with procArray->replication_slot_catalog_xmin's value then update ShmemVariableCache->oldestCatalogXmin in shmem. ShmemVariableCache->oldestCatalogXmin will get written out in the next checkpoint, which gets incorporated in the control file. There is a problem though - StartupReplicationSlots() and RestoreSlotFromDisk() don't care if catalog_xmin is set on a slot but wal_level is < logical and will happily restore a logical slot, or a physical slot with a catalog_xmin. So we can't actually assume that procArray->replication_slot_catalog_xmin will be 0 if we're not writing new logical WAL. This isn't a big deal, it just means we can't short-circuit UpdateOldestCatalogXmin() calls if !XLogLogicalInfoActive(). It also means the XLogReportParameters() stuff can be removed since we don't care about wal_level for tracking oldestCatalogXmin. Fixed in updated patch. I'm now reading over Andres's review. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
