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: CAPpHfdvN-_0VJ+0Cmb7PQN+p8PqMzvh+Fo0pW7Rj_xfpejAtgg@mail.gmail.com (view raw or flat)
Thread:
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:
> 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.

In response to

Responses

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-2014 The PostgreSQL Global Development Group