Re: Few observations in replication slots related code

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