| From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
|---|---|
| To: | Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> |
| Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, shveta malik <shvetamalik(at)gmail(dot)com> |
| Subject: | Re: Proposal: Conflict log history table for Logical Replication |
| Date: | 2026-06-02 05:21:03 |
| Message-ID: | CAFiTN-sqEMAbZ2pTt=zMa=918NV7HVeXF4bCOF+swtzKnTy5yQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, Jun 1, 2026 at 5:53 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Sat, May 30, 2026 at 1:42 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > In latest patch set I have fixed Nisha's comments by creating a toast
> > table, a separate patch
> > (v43-0005-Create-conflict-log-table-after-inserting-subscr.patch)
> > attached for creating conflict log table after inserting subscription
> > row.
> >
>
> Thanks for the patches. Please find a couple of comments on v43:
>
> 1) A non-superuser cannot read the new columns 'subconflictlogrelid'
> and 'subconflictlogdest' from pg_subscription.
> A comment in system_views.sql say:
> "-- All columns of pg_subscription except subconninfo are publicly readable."
>
> I think we should grant public access to these new columns as well.
>
> 2) patch-002: conflict.c
>
> -const ConflictLogColumnDef ConflictLogSchema[] = {
> +StaticAssertDecl(lengthof(ConflictLogDestNames) == 3,
> + "ConflictLogDestNames length mismatch");
> +
>
> Should we use "CONFLICT_LOG_DEST_ALL + 1" instead of the hard-coded value "3"?
>
> The attached diff fixes the above items, along with a few indentation
> and whitespace. Please consider it if you agree with the changes.
PFA, latest version of patch
Changes:
1. Includes Nisha's suggestion from v43_comments_nisha.patch
2. Include Amit's suggestion from v43-0001_amit.1.patch
3. Include Amit's suggestion: change GetLogDestination to
GetConflictLogDest(), drop_sub_conflict_log_table() to
drop_sub_conflict_log_table(), CONFLICTS_LOGGED_TO_FILE to
CONFLICTS_LOGGED_TO_LOG and renamed IsConflictNamespace() to
IsConflictLogTableNamespace()
4. Include Vignesh's fix Parallel_apply_failure.patch
5. Change conflict log table name back to conflict_log_table_<subid>
6. Changed acl to exclude INSERT/UPDATE/USAGE as it was there in initial patch
7. Merged 0002 to 0001 and 0004 to 0003 after review
Open
1. Vignesh please rebase upgrade and \dRs+ page
2. I will review and update the doc patch accordingly
--
Regards,
Dilip Kumar
Google
| Attachment | Content-Type | Size |
|---|---|---|
| v44-0002-Implement-the-conflict-insertion-infrastructure-.patch | application/octet-stream | 33.3 KB |
| v44-0001-Add-configurable-conflict-log-table-for-Logical-.patch | application/octet-stream | 67.5 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tatsuo Ishii | 2026-06-02 05:23:56 | Re: Row pattern recognition |
| Previous Message | Tatsuo Ishii | 2026-06-02 05:20:09 | Re: Row pattern recognition |