Horton

Thread

Commits

  1. Fix formatting and comment typos

  2. Fix assorted doc typos

  3. Clarify DROP EXTENSION docs regarding explicitly dependent routines

  4. doc: clarify how dropping of extensions affects dependent objs.

  5. Move remaining code from tqual.[ch] to heapam.h / heapam_visibility.c.

  1. fix typos

    Erik Rijkers <er@xs4all.nl> — 2022-08-01T18:04:54Z

    Recent typos...

  2. Re: fix typos

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-01T18:11:36Z

    On Mon, Aug 01, 2022 at 08:04:54PM +0200, Erik Rijkers wrote:
> Recent typos...

LGTM, thanks.

Here are some others I've been sitting on, mostly in .c files.

-- 
Justin

  3. Re: fix typos

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-02T05:28:21Z

    On Tue, Aug 2, 2022 at 1:05 AM Erik Rijkers <er@xs4all.nl> wrote:
>
> Recent typos...

The part of the sentence inside parentheses is not clear to me, before or
after the patch:

    Dropping an extension causes its component objects, and other explicitly
    dependent routines (see <xref linkend="sql-alterroutine"/>,
-   the depends on extension action), to be dropped as well.
+   that depend on extension action), to be dropped as well.
   </para>

--
John Naylor
EDB: http://www.enterprisedb.com

  4. Re: fix typos

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-02T05:52:53Z

    On Tue, Aug 2, 2022 at 1:11 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> Here are some others I've been sitting on, mostly in .c files.

0002:
weird since c91560defc57f89f7e88632ea14ae77b5cec78ee

It was weird long before that, maybe we should instead change most of those
tabs in the top comment to single space, as is customary? The rest LGTM.

--
John Naylor
EDB: http://www.enterprisedb.com

  5. Re: fix typos

    Erik Rijkers <er@xs4all.nl> — 2022-08-02T08:32:15Z

    Op 02-08-2022 om 07:28 schreef John Naylor:
> 
> On Tue, Aug 2, 2022 at 1:05 AM Erik Rijkers <er@xs4all.nl 
> <mailto:er@xs4all.nl>> wrote:
>  >
>  > Recent typos...
> 
> The part of the sentence inside parentheses is not clear to me, before 
> or after the patch:
> 
>      Dropping an extension causes its component objects, and other 
> explicitly
>      dependent routines (see <xref linkend="sql-alterroutine"/>,
> -   the depends on extension action), to be dropped as well.
> +   that depend on extension action), to be dropped as well.
>     </para>
> 

Hm, I see what you mean, I did not notice that earlier and I won't make 
a guess as to intention.  Maybe Bruce can have another look? (commit 
5fe2d4c56e)


> --
> John Naylor
> EDB: http://www.enterprisedb.com <http://www.enterprisedb.com>

  6. Re: fix typos

    Robert Haas <robertmhaas@gmail.com> — 2022-08-03T16:41:33Z

    On Tue, Aug 2, 2022 at 4:32 AM Erik Rijkers <er@xs4all.nl> wrote:
> > The part of the sentence inside parentheses is not clear to me, before
> > or after the patch:
> >
> >      Dropping an extension causes its component objects, and other
> > explicitly
> >      dependent routines (see <xref linkend="sql-alterroutine"/>,
> > -   the depends on extension action), to be dropped as well.
> > +   that depend on extension action), to be dropped as well.
> >     </para>
> >
>
> Hm, I see what you mean, I did not notice that earlier and I won't make
> a guess as to intention.  Maybe Bruce can have another look? (commit
> 5fe2d4c56e)

I think that it's talking about this (documented) syntax:

ALTER ROUTINE name [ ( [ [ argmode ] [ argname ] argtype [, ...] ] ) ]
    [ NO ] DEPENDS ON EXTENSION extension_name

So the change from "depends" to "depend" here is incorrect. Maybe we
can say something like:

the <literal>DEPENDS ON EXTENSION
<replaceable>extension_name</replaceable><literal> action

(I haven't tested whether this markup works.)

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  7. Re: fix typos

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-04T08:08:59Z

    On Wed, Aug 3, 2022 at 11:41 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> I think that it's talking about this (documented) syntax:
>
> ALTER ROUTINE name [ ( [ [ argmode ] [ argname ] argtype [, ...] ] ) ]
>     [ NO ] DEPENDS ON EXTENSION extension_name
>
> So the change from "depends" to "depend" here is incorrect. Maybe we
> can say something like:
>
> the <literal>DEPENDS ON EXTENSION
> <replaceable>extension_name</replaceable><literal> action
>
> (I haven't tested whether this markup works.)

Makes sense, I'll go make it happen.

--
John Naylor
EDB: http://www.enterprisedb.com

  8. Re: fix typos

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-04T10:21:38Z

    On Tue, Aug 2, 2022 at 1:11 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Aug 01, 2022 at 08:04:54PM +0200, Erik Rijkers wrote:
> > Recent typos...
>
> LGTM, thanks.
>
> Here are some others I've been sitting on, mostly in .c files.

I pushed Robert's suggestion, then pushed the rest of Erik's changes and
two of Justin's. For Justin's 0004:

--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -364,7 +364,7 @@ restart:
  if (nowait)
  ereport(ERROR,
  (errcode(ERRCODE_OBJECT_IN_USE),
- errmsg("could not drop replication origin with OID %d, in use by PID %d",
+ errmsg("could not drop replication origin with OID %u, in use by PID %d",

RepOriginId is a typedef for uint16, so this can't print the wrong answer,
but it is inconsistent with other uses. So it seems we don't need to
backpatch this one?

For patch 0002, the whitespace issue in the top comment in inval.c, I'm
inclined to just change all the out-of-place tabs in a single commit, so we
can add that to the list of whitespace commits.

--
John Naylor
EDB: http://www.enterprisedb.com

  9. Re: fix typos

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-04T13:41:05Z

    John Naylor <john.naylor@enterprisedb.com> writes:
> On Tue, Aug 2, 2022 at 1:11 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>   ereport(ERROR,
>   (errcode(ERRCODE_OBJECT_IN_USE),
> - errmsg("could not drop replication origin with OID %d, in use by PID %d",
> + errmsg("could not drop replication origin with OID %u, in use by PID %d",

> RepOriginId is a typedef for uint16, so this can't print the wrong answer,
> but it is inconsistent with other uses. So it seems we don't need to
> backpatch this one?

Um ... if it's int16, then it can't be an OID, so I'd say this message has
far worse problems than %d vs %u.  It should not use that terminology.

			regards, tom lane

  10. Re: fix typos

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-05T04:47:23Z

    On Thu, Aug 4, 2022 at 8:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> John Naylor <john.naylor@enterprisedb.com> writes:

> > RepOriginId is a typedef for uint16, so this can't print the wrong
answer,
> > but it is inconsistent with other uses. So it seems we don't need to
> > backpatch this one?
>
> Um ... if it's int16, then it can't be an OID, so I'd say this message has
> far worse problems than %d vs %u.  It should not use that terminology.

The catalog has the following. Since it's not a real oid, maybe this column
should be rethought?

CATALOG(pg_replication_origin,6000,ReplicationOriginRelationId)
BKI_SHARED_RELATION
{
/*
* Locally known id that get included into WAL.
*
* This should never leave the system.
*
* Needs to fit into an uint16, so we don't waste too much space in WAL
* records. For this reason we don't use a normal Oid column here, since
* we need to handle allocation of new values manually.
*/
Oid roident;
[...]

--
John Naylor
EDB: http://www.enterprisedb.com

  11. Re: fix typos

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-12T06:59:30Z

    I wrote:

> On Thu, Aug 4, 2022 at 8:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > John Naylor <john.naylor@enterprisedb.com> writes:
>
> > > RepOriginId is a typedef for uint16, so this can't print the wrong answer,
> > > but it is inconsistent with other uses. So it seems we don't need to
> > > backpatch this one?
> >
> > Um ... if it's int16, then it can't be an OID, so I'd say this message has
> > far worse problems than %d vs %u.  It should not use that terminology.
>
> The catalog has the following. Since it's not a real oid, maybe this column should be rethought?

This is really a straw-man proposal, since I'm not volunteering to do
the work, or suggest anybody else should do the same. That being the
case, it seems we should just go ahead with Justin's patch for
consistency. Possibly we could also change the messages to say "ID"?

> CATALOG(pg_replication_origin,6000,ReplicationOriginRelationId) BKI_SHARED_RELATION
> {
> /*
> * Locally known id that get included into WAL.
> *
> * This should never leave the system.
> *
> * Needs to fit into an uint16, so we don't waste too much space in WAL
> * records. For this reason we don't use a normal Oid column here, since
> * we need to handle allocation of new values manually.
> */
> Oid roident;

-- 
John Naylor
EDB: http://www.enterprisedb.com

  12. Re: fix typos

    Euler Taveira <euler@eulerto.com> — 2022-08-12T12:04:21Z

    On Fri, Aug 12, 2022, at 3:59 AM, John Naylor wrote:
> This is really a straw-man proposal, since I'm not volunteering to do
> the work, or suggest anybody else should do the same. That being the
> case, it seems we should just go ahead with Justin's patch for
> consistency. Possibly we could also change the messages to say "ID"?
... or say

        could not drop replication origin %u, in use by PID %d

AFAICS there is no "with ID" but there is "with identifier". I personally
prefer to omit these additional words; it seems clear without them.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

  13. Re: fix typos

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-12T13:55:19Z

    John Naylor <john.naylor@enterprisedb.com> writes:
> This is really a straw-man proposal, since I'm not volunteering to do
> the work, or suggest anybody else should do the same. That being the
> case, it seems we should just go ahead with Justin's patch for
> consistency. Possibly we could also change the messages to say "ID"?

I'd be content if we change the user-facing messages (and documentation
if any) to say "ID" not "OID".

			regards, tom lane

  14. Re: fix typos

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-16T01:48:27Z

    On Fri, Aug 12, 2022 at 8:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> John Naylor <john.naylor@enterprisedb.com> writes:
> > This is really a straw-man proposal, since I'm not volunteering to do
> > the work, or suggest anybody else should do the same. That being the
> > case, it seems we should just go ahead with Justin's patch for
> > consistency. Possibly we could also change the messages to say "ID"?
>
> I'd be content if we change the user-facing messages (and documentation
> if any) to say "ID" not "OID".

The documentation has both, so it makes sense to standardize on "ID".
The messages all had oid/OID, which I changed in the attached. I think
I got all the places.

I'm thinking it's not wrong enough to confuse people, but consistency
is good, so backpatch to v15 and no further. Does anyone want to make
a case otherwise?

-- 
John Naylor
EDB: http://www.enterprisedb.com

  15. Re: fix typos

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-18T02:11:55Z

    On Tue, Aug 16, 2022 at 8:48 AM John Naylor
<john.naylor@enterprisedb.com> wrote:
>
> On Fri, Aug 12, 2022 at 8:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > John Naylor <john.naylor@enterprisedb.com> writes:
> > > This is really a straw-man proposal, since I'm not volunteering to do
> > > the work, or suggest anybody else should do the same. That being the
> > > case, it seems we should just go ahead with Justin's patch for
> > > consistency. Possibly we could also change the messages to say "ID"?
> >
> > I'd be content if we change the user-facing messages (and documentation
> > if any) to say "ID" not "OID".
>
> The documentation has both, so it makes sense to standardize on "ID".
> The messages all had oid/OID, which I changed in the attached. I think
> I got all the places.
>
> I'm thinking it's not wrong enough to confuse people, but consistency
> is good, so backpatch to v15 and no further. Does anyone want to make
> a case otherwise?

This is done.

-- 
John Naylor
EDB: http://www.enterprisedb.com