Re: Few observations in replication slots related code - Mailing list pgsql-hackers
From | Andres Freund |
---|---|
Subject | Re: Few observations in replication slots related code |
Date | |
Msg-id | 20140612071508.GY8406@alap3.anarazel.de Whole thread Raw |
In response to | Few observations in replication slots related code (Amit Kapila <amit.kapila16@gmail.com>) |
Responses |
Re: Few observations in replication slots related code
(Andres Freund <andres@2ndquadrant.com>)
Re: Few observations in replication slots related code (Amit Kapila <amit.kapila16@gmail.com>) |
List | pgsql-hackers |
Hi Amit, On 2014-06-12 08:55:59 +0530, Amit Kapila wrote: > 1. In function StartupReplicationSlots(XLogRecPtr checkPointRedo), > parameter checkPointRedo is not used. It used to be included in a debug message. Apparently the message was removed at some point (don't remember it, but I have a memory like a sieve). > 2. Few check are in different order in functions > pg_create_physical_replication_slot() and > pg_create_logical_replication_slot(). > > if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) > elog(ERROR, "return type must be a row type"); > check_permissions(); > > CheckLogicalDecodingRequirements(); > > I don't think it makes any difference, however having checks in similar > order seems better unless there is a reason for not doing so. Can change it. > 3. Currently there is any Assert (Assert(!MyReplicationSlot);) in > pg_create_logical_replication_slot(), is there a need for similar > Assert in pg_create_physical_replication_slot()? Hm. Shouldn't really matter either way these days, but I guess it doesn't hurt to add one. > 4. > StartupDecodingContext() > { > .. > context = AllocSetContextCreate(CurrentMemoryContext, > "Changeset Extraction Context", > } > > To be consistent, shall we name this context as > logical | logical decoding? Heh. That apparently escaped when things were renamed. Yes. > 5. > pg_create_logical_replication_slot() > { > .. > CreateInitDecodingContext() > > .. > ReplicationSlotPersist() > } > > Function pg_create_logical_replication_slot() is trying to > save slot twice once during CreateInitDecodingContext() and > then in ReplicationSlotPersist(), isn't it better if we can make > it do it just once? Doesn't work here. In the first save it's not yet marked as persistent - but we still need to safely reserve the xmin. It's also not something that should happen very frequently, so I'm not worried about the overhead. > 6. > elog(ERROR, "cannot handle changeset extraction yet"); > > Shouldn't it be better to use logical replication instead > of changeset extraction? Will change. > 7. > + * XXX: It might make sense to introduce ephemeral slots and always use > + * the slot mechanism. > > Already there are RS_EPHEMERAL slots, might be this > comment needs bit of rephrasing. > 8. Is there a chance of inconsistency, if pg_restxlog resets the > xlog and after restart, one of the slots contains xlog position > older than what is resetted by pg_resetxlog? Yes. There's lots of ways to screw over your database by using pg_resetxlog. I personally think there should be a required --yes-i-am-breaking-my-database parameter for pg_resetxlog. > 9. > void > LogicalIncreaseXminForSlot(XLogRecPtr current_lsn, TransactionId xmin) > { > .. > /* > * don't overwrite if we already have a newer xmin. This can happen if we > * restart decoding in a slot. > */ > if (TransactionIdPrecedesOrEquals(xmin, slot->data.catalog_xmin)) > { > } > .. > } > > Should we just release spinlock in this loop and just return, > rather than keeping no action loop? Don't think that'd make it any faster/simpler. We'd be stuck with duplicate codepaths. > 10. > * Iff ri_type = REPLICA_IDENTITY_INDEX, indexOid must be the Oid of a > suitable > * index. Otherwise, it should be InvalidOid. > */ > static void > relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, > bool is_internal) > > typo - *Iff* iff := if and only if. Thanks for the look. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
pgsql-hackers by date: