Re: Fix a comment error in logicalrep_write_typ()
On Thu, Jul 11, 2024 at 4:35 PM cca5507 wrote: > > Thank you for review! > > The commitfest link for tracking: > https://commitfest.postgresql.org/49/5121/ > I've pushed and closed the CF entry. -- With Regards, Amit Kapila.
Re: Fix a comment error in logicalrep_write_typ()
Thank you for review! The commitfest link for tracking: https://commitfest.postgresql.org/49/5121/ -- Regards, ChangAo Chen
Re: Fix a comment error in logicalrep_write_typ()
On Thu, Jul 11, 2024 at 12:46 PM cca5507 wrote: > > - /* use Oid as relation identifier */ > + /* use Oid as type identifier */ > pq_sendint32(out, typoid); > > I think it must be "type identifier" rather than "relation identifier". > LGTM. -- With Regards, Amit Kapila.
Re: Fix a comment on PQcancelErrorMessage
On 2024-Jul-04, Yugo NAGATA wrote: > On Thu, 4 Jul 2024 11:06:03 +0200 > Jelte Fennema-Nio wrote: > > > On Thu, 4 Jul 2024 at 06:46, Yugo NAGATA wrote: > > > Attached is a small patch to fix a comment on PQcancelErrorMessage. > > > > Oops, copy paste mistake on my part I guess. New comment LGTM > > Thank you for your comments. > > I made a trivial change that fixes the line break position in keeping with > the surrounding comments. Ugh, mea culpa for failing to notice -- thank you, pushed. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Learn about compilers. Then everything looks like either a compiler or a database, and now you have two problems but one of them is fun." https://twitter.com/thingskatedid/status/1456027786158776329
Re: Fix a comment on PQcancelErrorMessage
On Thu, 4 Jul 2024 11:06:03 +0200 Jelte Fennema-Nio wrote: > On Thu, 4 Jul 2024 at 06:46, Yugo NAGATA wrote: > > Attached is a small patch to fix a comment on PQcancelErrorMessage. > > Oops, copy paste mistake on my part I guess. New comment LGTM Thank you for your comments. I made a trivial change that fixes the line break position in keeping with the surrounding comments. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/src/interfaces/libpq/fe-cancel.c b/src/interfaces/libpq/fe-cancel.c index 9562a7fe44..213a6f43c2 100644 --- a/src/interfaces/libpq/fe-cancel.c +++ b/src/interfaces/libpq/fe-cancel.c @@ -299,7 +299,8 @@ PQcancelSocket(const PGcancelConn *cancelConn) /* * PQcancelErrorMessage * - * Get the socket of the cancel connection. + * Returns the error message most recently generated by an operation on the + * cancel connection. */ char * PQcancelErrorMessage(const PGcancelConn *cancelConn)
Re: Fix a comment on PQcancelErrorMessage
On Thu, 4 Jul 2024 at 06:46, Yugo NAGATA wrote: > Attached is a small patch to fix a comment on PQcancelErrorMessage. Oops, copy paste mistake on my part I guess. New comment LGTM
Re: Fix a comment on PQcancelErrorMessage
> Hi, > > Attached is a small patch to fix a comment on PQcancelErrorMessage. > > It was accidentally "Get the socket of the cancel connection." > I rewrote it to "Returns the error message most recently generated > by an operation on the cancel connection." Good catch. The proposed message matches the PQCancelErrorMessage() documentation and looks good. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Fix a comment in basic_archive about NO_INSTALLCHECK
On Mon, Dec 18, 2023 at 11:28:46AM +0530, Bharath Rupireddy wrote: > Reworded the comment a bit and attached the v2 patch. Forgot about this one, thanks! I've simplified it a bit and applied it. -- Michael signature.asc Description: PGP signature
Re: Fix a comment in basic_archive about NO_INSTALLCHECK
On Thu, Apr 6, 2023 at 9:26 AM Michael Paquier wrote: > > On Mon, Apr 03, 2023 at 08:56:10AM +0530, Bharath Rupireddy wrote: > > It looks like comments in make file and meson file about not running > > basic_archive tests in NO_INSTALLCHECK mode are wrong. The comments say the > > module needs to be loaded via shared_preload_libraries=basic_archive, but > > it actually doesn't. The custom file needs archive related parameters and > > wal_level=replica. Here's a patch correcting that comment. > > Saying that, updating the comments about the dependency with > archive_library and the module's GUC is right. Reworded the comment a bit and attached the v2 patch. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com v2-0001-Fix-a-comment-in-basic_archive-about-NO_INSTALLCH.patch Description: Binary data
Re: Fix a comment in paraminfo_get_equal_hashops
On Fri, 4 Aug 2023 at 18:48, Richard Guo wrote: > As stated in [1], there is a typo in the comment in > paraminfo_get_equal_hashops. > [1] > https://www.postgresql.org/message-id/cambws49dehrpe8pom_k39r2uosaozcg+y0b5a8tf7vw3uvr...@mail.gmail.com Thanks. Pushed. David
Re: Fix a comment in basic_archive about NO_INSTALLCHECK
On Thu, Apr 6, 2023 at 9:26 AM Michael Paquier wrote: > > On Mon, Apr 03, 2023 at 08:56:10AM +0530, Bharath Rupireddy wrote: > > It looks like comments in make file and meson file about not running > > basic_archive tests in NO_INSTALLCHECK mode are wrong. The comments say the > > module needs to be loaded via shared_preload_libraries=basic_archive, but > > it actually doesn't. The custom file needs archive related parameters and > > wal_level=replica. Here's a patch correcting that comment. > > Wouldn't it be better to also set shared_preload_libraries in > basic_archive.conf? It is true that the test works fine if setting > only archive_library, which would cause the library with its > _PG_init() to be loaded in the archiver process. However the GUC > basic_archive.archive_directory is missing from the backends. Hm, I think the other backends will still see the value of the GUC without shared_preload_libraries=basic_archive. You can verify it with adding SHOW basic_archive.archive_directory; to basic_archive.sql. The basic_archive library gets loaded by archiver via _PG_init. It's the archiver defining a custom GUC variable which will propagate to all the postgres processes via set_config_option_ext. Therefore, we don't need shared_preload_libraries=basic_archive. #3 0x7f75306406b6 in _PG_init () at basic_archive.c:86 #4 0x562652d0c87c in internal_load_library ( libname=0x5626549102d8 "/home/ubuntu/postgres/tmp_install/home/ubuntu/postgres/inst/lib/basic_archive.so") at dfmgr.c:289 #5 0x562652d0c1e7 in load_external_function (filename=0x562654930698 "basic_archive", funcname=0x562652eca81b "_PG_archive_module_init", signalNotFound=false, filehandle=0x0) at dfmgr.c:116 #6 0x562652a3a400 in LoadArchiveLibrary () at pgarch.c:841 #7 0x562652a39489 in PgArchiverMain () at pgarch.c:256 #8 0x562652a353de in AuxiliaryProcessMain (auxtype=ArchiverProcess) at auxprocess.c:145 #9 0x562652a40b8e in StartChildProcess (type=ArchiverProcess) at postmaster.c:5341 #10 0x562652a3e529 in process_pm_child_exit () at postmaster.c:3072 #11 0x562652a3c329 in ServerLoop () at postmaster.c:1767 #12 0x562652a3bc52 in PostmasterMain (argc=8, argv=0x56265490e1e0) at postmaster.c:1462 #13 0x5626528efbbf in main (argc=8, argv=0x56265490e1e0) at main.c:198 > Saying that, updating the comments about the dependency with > archive_library and the module's GUC is right. Thanks. Any thoughts on the v1 patch attached upthread? -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Fix a comment in basic_archive about NO_INSTALLCHECK
On Mon, Apr 03, 2023 at 08:56:10AM +0530, Bharath Rupireddy wrote: > It looks like comments in make file and meson file about not running > basic_archive tests in NO_INSTALLCHECK mode are wrong. The comments say the > module needs to be loaded via shared_preload_libraries=basic_archive, but > it actually doesn't. The custom file needs archive related parameters and > wal_level=replica. Here's a patch correcting that comment. Wouldn't it be better to also set shared_preload_libraries in basic_archive.conf? It is true that the test works fine if setting only archive_library, which would cause the library with its _PG_init() to be loaded in the archiver process. However the GUC basic_archive.archive_directory is missing from the backends. Saying that, updating the comments about the dependency with archive_library and the module's GUC is right. -- Michael signature.asc Description: PGP signature
Re: Fix a comment in WalSnd structure
On Fri, Aug 19, 2022 at 05:40:40PM +0530, Bharath Rupireddy wrote: > WalSnd structure mutex is being used to protect all the variables of > that structure, not just 'variables shown above' [1]. A tiny patch > attached to fix the comment. Yep, walsender.c tells the same story, aka that replyTime and latch are updated with the spinlock taken. I'll go update the comment, and you suggestion sounds fine to me. -- Michael signature.asc Description: PGP signature
Re: Fix a comment in worker.c
On Fri, Feb 18, 2022 at 1:48 PM Amit Kapila wrote:
>
> On Wed, Feb 16, 2022 at 4:56 PM Amit Kapila wrote:
> >
> > On Wed, Feb 16, 2022 at 5:57 AM Masahiko Sawada
> > wrote:
> > >
> > > While reading the code, I realized that the second sentence of the
> > > following comment in worker.c is not correct:
> > >
> > >/*
> > > * Exit if the subscription was disabled. This normally should not
> > > happen
> > > * as the worker gets killed during ALTER SUBSCRIPTION ... DISABLE.
> > > */
> > > if (!newsub->enabled)
> > > {
> > > ereport(LOG,
> > > (errmsg("logical replication apply worker for
> > > subscription \"%s\" will "
> > > "stop because the subscription was disabled",
> > > MySubscription->name)));
> > >
> > > proc_exit(0);
> > > }
> > >
> > > IIUC the apply worker normally exits here when the subscription is
> > > disabled since we don't stop the apply worker during ALTER
> > > SUBSCRIPTION DISABLE. I've attached a patch to remove it.
> > >
> >
> > Yes, I also have the same understanding. Your patch LGTM. I'll push
> > this unless someone thinks otherwise.
> >
>
> Pushed.
Thanks!
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Re: Fix a comment in worker.c
On Wed, Feb 16, 2022 at 4:56 PM Amit Kapila wrote:
>
> On Wed, Feb 16, 2022 at 5:57 AM Masahiko Sawada wrote:
> >
> > While reading the code, I realized that the second sentence of the
> > following comment in worker.c is not correct:
> >
> >/*
> > * Exit if the subscription was disabled. This normally should not happen
> > * as the worker gets killed during ALTER SUBSCRIPTION ... DISABLE.
> > */
> > if (!newsub->enabled)
> > {
> > ereport(LOG,
> > (errmsg("logical replication apply worker for
> > subscription \"%s\" will "
> > "stop because the subscription was disabled",
> > MySubscription->name)));
> >
> > proc_exit(0);
> > }
> >
> > IIUC the apply worker normally exits here when the subscription is
> > disabled since we don't stop the apply worker during ALTER
> > SUBSCRIPTION DISABLE. I've attached a patch to remove it.
> >
>
> Yes, I also have the same understanding. Your patch LGTM. I'll push
> this unless someone thinks otherwise.
>
Pushed.
--
With Regards,
Amit Kapila.
Re: Fix a comment in worker.c
On Wed, Feb 16, 2022 at 5:57 AM Masahiko Sawada wrote:
>
> While reading the code, I realized that the second sentence of the
> following comment in worker.c is not correct:
>
>/*
> * Exit if the subscription was disabled. This normally should not happen
> * as the worker gets killed during ALTER SUBSCRIPTION ... DISABLE.
> */
> if (!newsub->enabled)
> {
> ereport(LOG,
> (errmsg("logical replication apply worker for
> subscription \"%s\" will "
> "stop because the subscription was disabled",
> MySubscription->name)));
>
> proc_exit(0);
> }
>
> IIUC the apply worker normally exits here when the subscription is
> disabled since we don't stop the apply worker during ALTER
> SUBSCRIPTION DISABLE. I've attached a patch to remove it.
>
Yes, I also have the same understanding. Your patch LGTM. I'll push
this unless someone thinks otherwise.
--
With Regards,
Amit Kapila.
Re: fix a comment
On Sat, Apr 24, 2021 at 11:43 AM Michael Paquier wrote: > > On Fri, Apr 23, 2021 at 07:03:40AM +, wangyu...@fujitsu.com wrote: > > Agree. Please see the v2 patch which delete the number in comment. > > Indeed, this set of comments became a bit obsolete after 1375422, as > you saied upthread. This simplification looks fine to me, so > applied. I am in a mood for such patches since yesterday.. :) Thank you ! Regards, Amul
Re: fix a comment
On Fri, Apr 23, 2021 at 07:03:40AM +, wangyu...@fujitsu.com wrote: > Agree. Please see the v2 patch which delete the number in comment. Indeed, this set of comments became a bit obsolete after 1375422, as you saied upthread. This simplification looks fine to me, so applied. I am in a mood for such patches since yesterday.. -- Michael signature.asc Description: PGP signature
RE: fix a comment
Hi, Amul Thank you for reviewing. > How about simply removing these numbering? Agree. Please see the v2 patch which delete the number in comment. Best wishes Yukun Wang -Original Message- From: Amul Sul Sent: Friday, April 23, 2021 3:51 PM To: Wang, Yukun/王 俞坤 Cc: pgsql-hack...@postgresql.org Subject: Re: fix a comment On Fri, Apr 23, 2021 at 12:12 PM wangyu...@fujitsu.com wrote: > > Hi, Hackers: > > In function ExecGetTriggerResultRel, we can see comments: > > > /* First, search through the query result relations */ ... > > /* > > * Third, search through the result relations that were created > > during > > * tuple routing, if any. > > */ > > But the 'Second' was deleted since commit 1375422c78. > > Update the 'Third' to 'Second', please see the attachment. > > Thoughts? > Well yes, looks good to me. How about simply removing these numbering? Regards, Amul fix-a-comment.diff Description: fix-a-comment.diff
Re: fix a comment
On Fri, Apr 23, 2021 at 12:12 PM wangyu...@fujitsu.com wrote: > > Hi, Hackers: > > In function ExecGetTriggerResultRel, we can see comments: > > > /* First, search through the query result relations */ ... > > /* > > * Third, search through the result relations that were created during > > * tuple routing, if any. > > */ > > But the 'Second' was deleted since commit 1375422c78. > > Update the 'Third' to 'Second', please see the attachment. > > Thoughts? > Well yes, looks good to me. How about simply removing these numbering? Regards, Amul
Re: Fix a comment in CreateLockFile
On Sun, Dec 8, 2019 at 4:32 PM Amit Kapila wrote: > > On Sun, Dec 8, 2019 at 1:10 PM Hadi Moshayedi wrote: > > > > It seems that explanation for the contents of the pid file has moved to > > pidfile.h, but the comments in CreateLockFile() still point to miscadmin.h. > > > > The attached patch updates those pointers. > > > > Your patch looks correct to me on a quick look. I will take of this tomorrow. > Pushed. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Fix a comment in CreateLockFile
On Sun, Dec 8, 2019 at 1:10 PM Hadi Moshayedi wrote: > > It seems that explanation for the contents of the pid file has moved to > pidfile.h, but the comments in CreateLockFile() still point to miscadmin.h. > > The attached patch updates those pointers. > Your patch looks correct to me on a quick look. I will take of this tomorrow. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
