Re: logical changeset generation v6.1

Hi,

On 2013-10-01 10:07:19 -0400, Robert Haas wrote:
> - AssignTransactionId changes "Mustn't" to "May not", which seems like
> an entirely pointless change.

It was "Musn't" before ;). I am not sure why I changed it to "May not"
instead of "Mustn't".

> - Do none of the callers of IsSystemRelation() care about the fact
> that you've considerably changed the semantics?

Afaics no. I think the semantics are actually consistent until somebody
manually creates a relation in pg_catalog (using allow_...). And in that
case the new semantics actually seem more useful.

> - RelationIsDoingTimetravel is still a crappy name. How about
> RelationRequiredForLogicalDecoding? And maybe the reloption
> treat_as_catalog_table can become required_for_logical_decoding.

Fine with me.

> - I don't understand the comment in xl_heap_new_cid to the effect that
> the combocid isn't needed for decoding. How not?

We don't use the combocid for antything - since we have the original
cmin/cmax, we can just use those and ignore the value of the combocid itself.

> - xlogreader.h includes an additional header with no other changes.
> Doesn't seem right.

Hm. I seem to remember having a reason for that, but for the heck can't
see it anymore...

> I have no problem with caching the primary key in the relcache, or
> with using that as the default key for logical decoding, but I'm
> extremely uncomfortable with the fallback strategy when no primary key
> exists. Choosing any old unique index that happens to present itself
> as the primary key feels wrong to me.
> [stuff I don't disagree with]

People lobbied vigorously to allow candidate keys before. I personally
would never want to use anything but an actual primary key for
replication, but there's other usecases than replication.

I think it's going to be the domain of the replication solution to
enforce the presence of primary keys. I.e. they should (be able to) use
event triggers or somesuch to enforce it...

> All in all, it seems to me that we shouldn't try to punt. Maybe we
> should have something that works like ALTER TABLE name CLUSTER ON
> index_name to configure which index should be used for logical
> replication. Possibly this same syntax could be used as ALTER
> MATERIALIZED VIEW to set the candidate key for that case.

I'd be fine with that, but I am also not particularly interested in it
because I personally don't see much of a usecase.
For replication ISTM the only case where there would be no primary key
is a) initial load b) replacing the primary key by another index.

> What happens if new unique indexes are created or old ones dropped
> while logical replication is running?

Should just work, but I'll make sure the tests cover this.

The output plugin needs to lookup the current index used, and it will
use a consistent syscache state and thus will find the same index.
In bdr the output plugin simply includes the name of the index used in
the replication stream to make sure things are somewhat consistent.

Will fix or think about the rest.

Thanks,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services