Re: [PATCH 14/16] Add module to apply changes from an apply-cache using low-level functions

Hi, Andres!

There is my review of this patch.

1) Patches don't apply cleanly to head. So I used commit
bed88fceac04042f0105eb22a018a4f91d64400d as the base for patches, then all
the patches close to this apply cleanly. Regression tests pass OK, but it
seems that new functionality isn't covered by regression tests.

2) Patch needs more comments. I think we need at least one comment in head
of each function describing its behaviour, even if it is evident from
function name.

4) There is significant code duplication in APPLY_CACHE_CHANGE_UPDATE and
APPLY_CACHE_CHANGE_DELETE branches of case in apply_change function. I
think this could be refactored to reduce code duplication.

5) Apply mechanism requires PK from each table. So, throwing error here if
we don't find PK is necessary. But we need to prevent user from run logical
replication earlier than failing applying received messages. AFACS patch
which is creating corresponding log messages is here:
http://archives.postgresql.org/message-id/1339586927-13156-7-git-send-email-andres@2ndquadrant.com.
And it throws any warning if it fails to find PK. On which stage we prevent
user from running logical replication on tables which doesn't have PK?

6) I've seen comment /* FIXME: locking */. But you open index with command
index_rel = index_open(indexoid, AccessShareLock);
and close it with command
heap_close(index_rel, NoLock);
Shouldn't we use same level of locking on close as on open? Also,
heap_close doesn't looks good to me for closing index. Why don't use
index_close or relation_close?

7) We find each updated and deleted tuple by PK. Imagine we update
significant part of the table (for example, 10%) in single query and
planner choose sequential scan for it. Then applying of this changes could
be more expensive than doing original changes. This it probably ok. But, we
could do some heuristics: detect that sequential scan is cheaper because of
large amount of updates or deletes in one table.

------
With best regards,
Alexander Korotkov.