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

On Sun, Jul 1, 2012 at 3:11 PM, Alexander Korotkov <aekorotkov(at)gmail(dot)com>wrote:

> 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.
>

8) If we can't find tuple for update or delete we likely need to put PK
value into the log message.

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