From: | Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com> |
---|---|
To: | shveta malik <shveta(dot)malik(at)gmail(dot)com> |
Cc: | "wangw(dot)fnst(at)fujitsu(dot)com" <wangw(dot)fnst(at)fujitsu(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication |
Date: | 2023-02-01 11:44:22 |
Message-ID: | CAGPVpCQ9VQrC4L6703h3AmkCO-+5DkgGWzNCUCdH1vFT=ApCkQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
I mistakenly attached v9 in my previous email.
Please see attached v6 and v10 for the previous and below changes.
shveta malik <shveta(dot)malik(at)gmail(dot)com>, 31 Oca 2023 Sal, 12:59 tarihinde
şunu yazdı:
> On Fri, Jan 27, 2023 at 3:41 PM shveta malik <shveta(dot)malik(at)gmail(dot)com>
> wrote:
> 1)
> REPLICATION_SLOT_SNAPSHOT
> --Do we need 'CREATE' prefix with it i.e. CREATE_REPLICATION_SNAPSHOT
> (or some other brief one with CREATE?). 'REPLICATION_SLOT_SNAPSHOT'
> does not look like a command/action and thus is confusing.
>
Renamed it as CREATE_REPLICATION_SNAPSHOT
> 2)
> is used in the currenct transaction. This command is currently only
> supported
> for logical replication.
> slots.
> --typo: currenct-->current
> --slots can be moved to previous line
>
Done.
> 3)
> /*
> * Signal that we don't need the timeout mechanism. We're just creating
> * the replication slot and don't yet accept feedback messages or send
> * keepalives. As we possibly need to wait for further WAL the walsender
> * would otherwise possibly be killed too soon.
> */
> We're just creating the replication slot --> We're just creating the
> replication snapshot
>
Done.
> 4)
> I see XactReadOnly check in CreateReplicationSlot, do we need the same
> in ReplicationSlotSnapshot() as well?
>
Added this check too.
> ===============
> v9-0002:
>
> 5)
> /* We are safe to drop the replication trackin origin after this
> --typo: tracking
>
Done.
> 6)
> slot->data.catalog_xmin = xmin_horizon;
> slot->effective_xmin = xmin_horizon;
> SpinLockRelease(&slot->mutex);
> xmin_horizon =
> GetOldestSafeDecodingTransactionId(!need_full_snapshot);
> ReplicationSlotsComputeRequiredXmin(true);
>
> --do we need to set xmin_horizon in slot after
> 'GetOldestSafeDecodingTransactionId' call, otherwise it will be set to
> InvalidId in slot. Is that intentional? I see that we do set this
> correct xmin_horizon in builder->initial_xmin_horizon but the slot is
> carrying Invalid one.
>
I think you're right. Moved GetOldestSafeDecodingTransactionId call before
xmin_horizon assignment.
Thanks,
--
Melih Mutlu
Microsoft
Attachment | Content-Type | Size |
---|---|---|
v6-0001-Add-replication-protocol-cmd-to-create-a-snapshot.patch | application/octet-stream | 20.9 KB |
v10-0002-Reuse-Logical-Replication-Background-worker.patch | application/octet-stream | 71.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Antonin Houska | 2023-02-01 11:45:57 | RLS makes COPY TO process child tables |
Previous Message | David Rowley | 2023-02-01 11:40:45 | Re: Can we do something to help stop users mistakenly using force_parallel_mode? |