Horton

Thread

Commits

  1. Remove the adminpack contrib extension

  1. Fix a typo in pg_rotate_logfile

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-02-12T20:32:21Z

    Hi,

I happened to notice a typo in pg_rotate_logfile in ipc/signalfuncs.c
- the hint message wrongly mentions that pg_logfile_rotate is part of
the core; which is actually not. pg_logfile_rotate is an adminpack's
1.0 SQL function dropped in 2.0. The core defines pg_rotate_logfile
SQL function instead, so use that. Here's a patch to fix the typo.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  2. Re: Fix a typo in pg_rotate_logfile

    Daniel Gustafsson <daniel@yesql.se> — 2024-02-12T20:39:06Z

    > On 12 Feb 2024, at 21:32, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

> I happened to notice a typo in pg_rotate_logfile in ipc/signalfuncs.c
> - the hint message wrongly mentions that pg_logfile_rotate is part of
> the core; which is actually not. pg_logfile_rotate is an adminpack's
> 1.0 SQL function dropped in 2.0. The core defines pg_rotate_logfile
> SQL function instead, so use that. Here's a patch to fix the typo.

Nice catch!  This needs to be backpatched all the way down to 12 as that
function wen't away a long time ago (it was marked as deprecated all the way
back in 9.1).

--
Daniel Gustafsson

  3. Re: Fix a typo in pg_rotate_logfile

    Nathan Bossart <nathandbossart@gmail.com> — 2024-02-12T20:46:13Z

    On Mon, Feb 12, 2024 at 09:39:06PM +0100, Daniel Gustafsson wrote:
>> On 12 Feb 2024, at 21:32, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>> I happened to notice a typo in pg_rotate_logfile in ipc/signalfuncs.c
>> - the hint message wrongly mentions that pg_logfile_rotate is part of
>> the core; which is actually not. pg_logfile_rotate is an adminpack's
>> 1.0 SQL function dropped in 2.0. The core defines pg_rotate_logfile
>> SQL function instead, so use that. Here's a patch to fix the typo.
> 
> Nice catch!  This needs to be backpatched all the way down to 12 as that
> function wen't away a long time ago (it was marked as deprecated all the way
> back in 9.1).

This is a bit strange because, with this patch, the HINT suggests using a
function with the same name as the one it lives in.  IIUC this is because
adminpack's pg_logfile_rotate() uses pg_rotate_logfile(), while core's
pg_rotate_logfile() uses pg_rotate_logfile_v2().  I suppose trying to
rename these might be more trouble than it's worth at this point, though...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  4. Re: Fix a typo in pg_rotate_logfile

    Daniel Gustafsson <daniel@yesql.se> — 2024-02-12T20:59:15Z

    > On 12 Feb 2024, at 21:46, Nathan Bossart <nathandbossart@gmail.com> wrote:
> 
> On Mon, Feb 12, 2024 at 09:39:06PM +0100, Daniel Gustafsson wrote:
>>> On 12 Feb 2024, at 21:32, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>>> I happened to notice a typo in pg_rotate_logfile in ipc/signalfuncs.c
>>> - the hint message wrongly mentions that pg_logfile_rotate is part of
>>> the core; which is actually not. pg_logfile_rotate is an adminpack's
>>> 1.0 SQL function dropped in 2.0. The core defines pg_rotate_logfile
>>> SQL function instead, so use that. Here's a patch to fix the typo.
>> 
>> Nice catch!  This needs to be backpatched all the way down to 12 as that
>> function wen't away a long time ago (it was marked as deprecated all the way
>> back in 9.1).
> 
> This is a bit strange because, with this patch, the HINT suggests using a
> function with the same name as the one it lives in.  IIUC this is because
> adminpack's pg_logfile_rotate() uses pg_rotate_logfile(), while core's
> pg_rotate_logfile() uses pg_rotate_logfile_v2().  I suppose trying to
> rename these might be more trouble than it's worth at this point, though...

Yeah, I doubt that's worth the churn.

On that note though, we might want to consider just dropping it altogether in
v17 (while fixing the incorrect hint in backbranches)?  I can't imagine
adminpack 1.0 being in heavy use today, and skimming pgAdmin code it seems it's
only used in pgAdmin3 and not 4. Maybe it's time to simply drop old code?

--
Daniel Gustafsson

  5. Re: Fix a typo in pg_rotate_logfile

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-02-12T21:31:39Z

    On Tue, Feb 13, 2024 at 2:29 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> On that note though, we might want to consider just dropping it altogether in
> v17 (while fixing the incorrect hint in backbranches)?  I can't imagine
> adminpack 1.0 being in heavy use today, and skimming pgAdmin code it seems it's
> only used in pgAdmin3 and not 4. Maybe it's time to simply drop old code?

https://codesearch.debian.net/search?q=pg_logfile_rotate&literal=1
shows no users for it though. There's pgadmin3 using it
https://github.com/search?q=repo%3Apgadmin-org%2Fpgadmin3%20pg_logfile_rotate&type=code,
however the repo is archived. Surprisingly, core has to maintain the
old code needed for adminpack 1.0 - pg_rotate_logfile_old SQL function
and pg_rotate_logfile function in signalfuncs.c. These things could
have been moved to adminpack.c back then and pointed CREATE FUNCTION
pg_catalog.pg_logfile_rotate() to use it from adminpack.c. If we
decide to remove adminpack 1.0 version completely, the 1.0 functions
pg_file_read, pg_file_length and pg_logfile_rotate will also go away
making adminpack code simpler.

Having said that, it's good to hear from others, preferably from
pgadmin developers - added Dave Page (dpage@pgadmin.org) in here for
inputs.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  6. Re: Fix a typo in pg_rotate_logfile

    Dave Page <dpage@pgadmin.org> — 2024-02-14T10:35:56Z

    Hi

On Mon, 12 Feb 2024 at 21:31, Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

> On Tue, Feb 13, 2024 at 2:29 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> >
> > On that note though, we might want to consider just dropping it
> altogether in
> > v17 (while fixing the incorrect hint in backbranches)?  I can't imagine
> > adminpack 1.0 being in heavy use today, and skimming pgAdmin code it
> seems it's
> > only used in pgAdmin3 and not 4. Maybe it's time to simply drop old code?
>
> https://codesearch.debian.net/search?q=pg_logfile_rotate&literal=1
> shows no users for it though. There's pgadmin3 using it
>
> https://github.com/search?q=repo%3Apgadmin-org%2Fpgadmin3%20pg_logfile_rotate&type=code
> ,
> however the repo is archived. Surprisingly, core has to maintain the
> old code needed for adminpack 1.0 - pg_rotate_logfile_old SQL function
> and pg_rotate_logfile function in signalfuncs.c. These things could
> have been moved to adminpack.c back then and pointed CREATE FUNCTION
> pg_catalog.pg_logfile_rotate() to use it from adminpack.c. If we
> decide to remove adminpack 1.0 version completely, the 1.0 functions
> pg_file_read, pg_file_length and pg_logfile_rotate will also go away
> making adminpack code simpler.
>
> Having said that, it's good to hear from others, preferably from
> pgadmin developers - added Dave Page (dpage@pgadmin.org) in here for
> inputs.
>

As it happens we're currently implementing a redesigned version of that
functionality from pgAdmin III in pgAdmin 4. However, we are not using
adminpack for it.

FWIW, the reason for the weird naming is that originally all the
functionality for reading/managing files was added entirely as the
adminpack extension. It was only later that some of the functionality was
moved into core, and renamed along the way (everyone likes blue for their
bikeshed right?). The old functions (albeit, rewritten to use the new core
functions) were kept in adminpack for backwards compatibility.

That said, pgAdmin III has been out of support for many years, and as far
as I know, it (and similarly old versions of EDB's PEM which was based on
it) were the only consumers of adminpack. I would not be sad to see it
removed entirely - except for the fact that I fondly remember being invited
to join -core immediately after a heated discussion with Tom about it!

-- 
Dave Page
pgAdmin: https://www.pgadmin.org
PostgreSQL: https://www.postgresql.org
EDB: https://www.enterprisedb.com

  7. Re: Fix a typo in pg_rotate_logfile

    Daniel Gustafsson <daniel@yesql.se> — 2024-02-14T13:35:39Z

    > On 14 Feb 2024, at 11:35, Dave Page <dpage@pgadmin.org> wrote:

> That said, pgAdmin III has been out of support for many years, and as far as I know, it (and similarly old versions of EDB's PEM which was based on it) were the only consumers of adminpack. I would not be sad to see it removed entirely

Searching on Github and Debian Codesearch I cannot find any reference to anyone
using any function from adminpack.  With pgAdminIII being EOL it might be to
remove it now rather than be on the hook to maintain it for another 5 years
until v17 goes EOL.  It'll still be around for years in V16->.

If anyone still uses pgAdminIII then I have a hard time believing they are
diligently updating to the latest major version of postgres..

Attached is a diff to show what it would look like to remove adminpack (catalog
version bump omitted on purpose to avoid conflicts until commit).

--
Daniel Gustafsson

  8. Re: Fix a typo in pg_rotate_logfile

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-14T15:04:49Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 14 Feb 2024, at 11:35, Dave Page <dpage@pgadmin.org> wrote:
>> That said, pgAdmin III has been out of support for many years, and as far as I know, it (and similarly old versions of EDB's PEM which was based on it) were the only consumers of adminpack. I would not be sad to see it removed entirely

> Searching on Github and Debian Codesearch I cannot find any reference to anyone
> using any function from adminpack.  With pgAdminIII being EOL it might be to
> remove it now rather than be on the hook to maintain it for another 5 years
> until v17 goes EOL.  It'll still be around for years in V16->.

Works for me.

> Attached is a diff to show what it would look like to remove adminpack (catalog
> version bump omitted on purpose to avoid conflicts until commit).

I don't see any references you missed, so +1.

			regards, tom lane

  9. Re: Fix a typo in pg_rotate_logfile

    Nathan Bossart <nathandbossart@gmail.com> — 2024-02-14T18:51:30Z

    On Wed, Feb 14, 2024 at 10:04:49AM -0500, Tom Lane wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 14 Feb 2024, at 11:35, Dave Page <dpage@pgadmin.org> wrote:
>>> That said, pgAdmin III has been out of support for many years, and as
>>> far as I know, it (and similarly old versions of EDB's PEM which was
>>> based on it) were the only consumers of adminpack. I would not be sad
>>> to see it removed entirely
> 
>> Searching on Github and Debian Codesearch I cannot find any reference to anyone
>> using any function from adminpack.  With pgAdminIII being EOL it might be to
>> remove it now rather than be on the hook to maintain it for another 5 years
>> until v17 goes EOL.  It'll still be around for years in V16->.
> 
> Works for me.
> 
>> Attached is a diff to show what it would look like to remove adminpack (catalog
>> version bump omitted on purpose to avoid conflicts until commit).
> 
> I don't see any references you missed, so +1.

Seems reasonable to me, too.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  10. Re: Fix a typo in pg_rotate_logfile

    Daniel Gustafsson <daniel@yesql.se> — 2024-02-14T20:48:43Z

    > On 14 Feb 2024, at 19:51, Nathan Bossart <nathandbossart@gmail.com> wrote:
> 
> On Wed, Feb 14, 2024 at 10:04:49AM -0500, Tom Lane wrote:
>> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 14 Feb 2024, at 11:35, Dave Page <dpage@pgadmin.org> wrote:
>>>> That said, pgAdmin III has been out of support for many years, and as
>>>> far as I know, it (and similarly old versions of EDB's PEM which was
>>>> based on it) were the only consumers of adminpack. I would not be sad
>>>> to see it removed entirely
>> 
>>> Searching on Github and Debian Codesearch I cannot find any reference to anyone
>>> using any function from adminpack.  With pgAdminIII being EOL it might be to
>>> remove it now rather than be on the hook to maintain it for another 5 years
>>> until v17 goes EOL.  It'll still be around for years in V16->.
>> 
>> Works for me.
>> 
>>> Attached is a diff to show what it would look like to remove adminpack (catalog
>>> version bump omitted on purpose to avoid conflicts until commit).
>> 
>> I don't see any references you missed, so +1.
> 
> Seems reasonable to me, too.

Thanks!  I'll put this in the next CF to keep it open for comments a bit
longer, but will close it early in the CF.

--
Daniel Gustafsson

  11. Re: Fix a typo in pg_rotate_logfile

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-02-16T07:22:47Z

    On Thu, Feb 15, 2024 at 2:18 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> >>> Searching on Github and Debian Codesearch I cannot find any reference to anyone
> >>> using any function from adminpack.  With pgAdminIII being EOL it might be to
> >>> remove it now rather than be on the hook to maintain it for another 5 years
> >>> until v17 goes EOL.  It'll still be around for years in V16->.
> >>
> >> Works for me.
> >>
> >>> Attached is a diff to show what it would look like to remove adminpack (catalog
> >>> version bump omitted on purpose to avoid conflicts until commit).
> >>
> >> I don't see any references you missed, so +1.
> >
> > Seems reasonable to me, too.
>
> Thanks!  I'll put this in the next CF to keep it open for comments a bit
> longer, but will close it early in the CF.

LGTM.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  12. Re: Fix a typo in pg_rotate_logfile

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-04T20:04:14Z

    > On 14 Feb 2024, at 21:48, Daniel Gustafsson <daniel@yesql.se> wrote:
>> On 14 Feb 2024, at 19:51, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> On Wed, Feb 14, 2024 at 10:04:49AM -0500, Tom Lane wrote:
>>> Daniel Gustafsson <daniel@yesql.se> writes:

>>>> Attached is a diff to show what it would look like to remove adminpack (catalog
>>>> version bump omitted on purpose to avoid conflicts until commit).
>>> 
>>> I don't see any references you missed, so +1.
>> 
>> Seems reasonable to me, too.
> 
> Thanks!  I'll put this in the next CF to keep it open for comments a bit
> longer, but will close it early in the CF.

This has now been pushed, adminpack has left the building.

--
Daniel Gustafsson