Skip site navigation (1) Skip section navigation (2)

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

From: Alexander Korotkov <aekorotkov(at)gmail(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH 14/16] Add module to apply changes from an apply-cache using low-level functions
Date: 2012-07-01 15:51:54
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
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:
> 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.

In response to


pgsql-hackers by date

Next:From: Fujii MasaoDate: 2012-07-01 16:01:43
Subject: Re: [PATCH] Make pg_basebackup configure and start standby
Previous:From: Magnus HaganderDate: 2012-07-01 15:44:30
Subject: Re: [PATCH] Make pg_basebackup configure and start standby

Privacy Policy | About PostgreSQL
Copyright © 1996-2018 The PostgreSQL Global Development Group