| From: | Shubham Khanna <khannashubham1197(at)gmail(dot)com> |
|---|---|
| To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
| Cc: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Add support for specifying tables in pg_createsubscriber. |
| Date: | 2025-11-06 04:05:36 |
| Message-ID: | CAHv8RjK3V+JOC=dQCeQqvrvyzXcY7GHhsYae5NL3TR82bCXeSw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Thu, Nov 6, 2025 at 7:18 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Nov 5, 2025 at 3:42 PM Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Mon, Nov 3, 2025 at 12:55 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > Hi Shubham.
> > >
> > > A comment about the v17-0001.
> > >
> > > ======
> > > 1.
> > > + if (check_publication_exists(conn, dbinfo[i].pubname, dbinfo[i].dbname))
> > > + {
> > > + /* Reuse existing publication on publisher. */
> > > + pg_log_info("dry-run: would use existing publication \"%s\" in
> > > database \"%s\"",
> > > + dbinfo[i].pubname, dbinfo[i].dbname);
> > > + dbinfo[i].made_publication = false;
> > > + }
> > >
> > > Is that correct? Won't this code now unconditionally log with the
> > > "dry-run:" prefix, even when the tool is *not* doing a dry-run?
> > >
> > > I thought code would be something like:
> > >
> > > SUGGESTION #1 (if/else)
> > > /* Reuse existing publication on publisher. */
> > > if (dry_run)
> > > pg_log_info("dry-run: would use existing publication ...);
> > > else
> > > pg_log_info("use existing publication ...);
> > >
> > > ~~~
> > >
> > > OTOH, (since here is just an info message with no destructive
> > > operation) perhaps it would be harmless also to keep the original log
> > > message for both dry-run and normal mode.
> > >
> > > SUGGESTION #2 (do nothing)
> > > pg_log_info("use existing publication ...);
> > >
> > > ======
> >
> > Hi Peter,
> >
> > Thank you for your review and suggestions.
> > I agree with your reasoning regarding the logging behavior. I will
> > proceed with Suggestion #2 and retain the existing `pg_log_info("use
> > existing publication ...");` message for both dry-run and normal
> > modes. This message is purely informational and does not perform any
> > destructive action, making it suitable for both scenarios.
> > The attached patch includes these changes.
> >
>
> OK, but you did not do quite what you said you did.
>
> Notice that v18 has modified the message to "would use existing
> publication...", instead of just leaving it how it was in v16 ("use
> existing publication...").
>
> ======
Fixed.
The attached patch includes these changes.
Thanks and regards,
Shubham Khanna.
| Attachment | Content-Type | Size |
|---|---|---|
| v19-0001-Support-existing-publications-in-pg_createsubscr.patch | application/octet-stream | 12.9 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Amit Kapila | 2025-11-06 04:40:23 | Re: Logical Replication of sequences |
| Previous Message | Etsuro Fujita | 2025-11-06 03:41:09 | Re: Obsolete comment in ExecScanReScan() |