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
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:
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
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,
In response to
pgsql-hackers by date
|Next:||From: Robert Haas||Date: 2012-07-01 12:23:06|
|Subject: Re: Update on the spinlock->pthread_mutex patch
experimental: replace s_lock spinlock code with pthread_mutex on linux|
|Previous:||From: Boszormenyi Zoltan||Date: 2012-07-01 11:02:17|
|Subject: [PATCH] Make pg_basebackup configure and start standby|