Обсуждение: Re: Catalog version access

On Wed, Mar 3, 2021 at 8:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Vik Fearing <vik@postgresfriends.org> writes:
> > On 3/3/21 6:35 PM, Peter Eisentraut wrote:
> >> If what you want to know is whether a given binary can run against a
> >> given data directory then CATALOG_VERSION_NO isn't the only thing you
> >> need to check.  The full truth of this is in ReadControlFile().  The
> >> best way to get that answer is to start a server and see if it
> >> complains.  You can even grep the log for "It looks like you need to
> >> initdb.".
>
> > In that case, what would everyone think about a  `pg_ctl check`  option?
>
> The trouble with Peter's recipe is that it doesn't work if there is
> already a server instance running there (or at least I think we'll
> bitch about the existing postmaster first, maybe I'm wrong).  Now,
> that's not such a big problem for the use-case you were describing.
> But I bet if we expose this method as an apparently-general-purpose
> pg_ctl option, there'll be complaints.

Another option could be to provide a switch to the postmaster binary.
Using pg_config as originally suggested is risky because you might
pick up the wrong postmaster, but if you put it on the actual
postmaster binary you certainly know which one you're on... As this is
something that's primarily of interest to developers, it's also a bit
lower weight than having a "heavy" solution like an entire mode for
pg_ctl.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On 4/8/21, 2:58 AM, "Magnus Hagander" <magnus@hagander.net> wrote:
> Another option could be to provide a switch to the postmaster binary.
> Using pg_config as originally suggested is risky because you might
> pick up the wrong postmaster, but if you put it on the actual
> postmaster binary you certainly know which one you're on... As this is
> something that's primarily of interest to developers, it's also a bit
> lower weight than having a "heavy" solution like an entire mode for
> pg_ctl.

I was looking at the --check switch for the postgres binary recently
[0], and this sounds like something that might fit in nicely there.
In the attached patch, --check will also check the control file if one
exists.

Nathan

[0] https://postgr.es/m/0545F7B3-70C0-4DE8-8C85-EAFE6113B7EE%40amazon.com

On Mon, Aug 16, 2021 at 06:12:54PM +0000, Bossart, Nathan wrote:
> I was looking at the --check switch for the postgres binary recently
> [0], and this sounds like something that might fit in nicely there.
> In the attached patch, --check will also check the control file if one
> exists.

This would not work on a running postmaster as CreateDataDirLockFile()
is called beforehand, but we want this capability, no?

Abusing a bootstrap option for this purpose does not look like a good
idea, to be honest, especially for something only used internally now
and undocumented, but we want to use something aimed at an external
use with some documentation.  Using a separate switch would be more
adapted IMO.  Also, I think that we should be careful with the read of
the control file to avoid false positives?   We can rely on an atomic
read/write thanks to its maximum size of 512 bytes, but this looks
like a lot what has been done recently with postgres -C for runtime
GUCs, that *require* a read of the control file before grabbing the
values we are interested in.
--
Michael

Thanks for taking a look!

On 1/23/22, 7:31 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Mon, Aug 16, 2021 at 06:12:54PM +0000, Bossart, Nathan wrote:
>> I was looking at the --check switch for the postgres binary recently
>> [0], and this sounds like something that might fit in nicely there.
>> In the attached patch, --check will also check the control file if one
>> exists.
>
> This would not work on a running postmaster as CreateDataDirLockFile()
> is called beforehand, but we want this capability, no?

I was not under the impression this was a requirement, based on the
use-case discussed upthread [0].  

> Abusing a bootstrap option for this purpose does not look like a good
> idea, to be honest, especially for something only used internally now
> and undocumented, but we want to use something aimed at an external
> use with some documentation.  Using a separate switch would be more
> adapted IMO.

This is the opposite of what Magnus proposed earlier [1].  Do we need
a new pg_ctl option for this?  It seems like it is really only
intended for use by PostgreSQL developers, but perhaps there are other
use-cases I am not thinking of.  In any case, the pg_ctl option would
probably end up using --check (or another similar mode) behind the
scenes.

> Also, I think that we should be careful with the read of
> the control file to avoid false positives?   We can rely on an atomic
> read/write thanks to its maximum size of 512 bytes, but this looks
> like a lot what has been done recently with postgres -C for runtime
> GUCs, that *require* a read of the control file before grabbing the
> values we are interested in.

Sorry, I'm not following this one.  In my proposed patch, the control
file (if one exists) is read after CreateDataDirLockFile(), just like
PostmasterMain().

Nathan

[0] https://postgr.es/m/20210222022407.ecaygvx2ise6uwyz%40alap3.anarazel.de
[1] https://postgr.es/m/CABUevEySovaEDci7c0DXOrV6c7JzWqYzfVwOiRUJxMao%3D9seEw%40mail.gmail.com

On Mon, Jan 24, 2022 at 08:40:08PM +0000, Bossart, Nathan wrote:
> On 1/23/22, 7:31 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
>> On Mon, Aug 16, 2021 at 06:12:54PM +0000, Bossart, Nathan wrote:
>>> I was looking at the --check switch for the postgres binary recently
>>> [0], and this sounds like something that might fit in nicely there.
>>> In the attached patch, --check will also check the control file if one
>>> exists.
>>
>> This would not work on a running postmaster as CreateDataDirLockFile()
>> is called beforehand, but we want this capability, no?
>
> I was not under the impression this was a requirement, based on the
> use-case discussed upthread [0].

Hmm.  I got a different impression as of this one:
https://www.postgresql.org/message-id/3496407.1613955241@sss.pgh.pa.us
But I can see downthread that this is not the case.  Sorry for the
confusion.

>> Abusing a bootstrap option for this purpose does not look like a good
>> idea, to be honest, especially for something only used internally now
>> and undocumented, but we want to use something aimed at an external
>> use with some documentation.  Using a separate switch would be more
>> adapted IMO.
>
> This is the opposite of what Magnus proposed earlier [1].  Do we need
> a new pg_ctl option for this?  It seems like it is really only
> intended for use by PostgreSQL developers, but perhaps there are other
> use-cases I am not thinking of.  In any case, the pg_ctl option would
> probably end up using --check (or another similar mode) behind the
> scenes.

Based on the latest state of the thread, I am understanding that we
don't want a new option for pg_ctl for this feature, and using a
bootstrap's --check for this purpose is not a good idea IMO.  What I
guess from Magnus' suggestion would be to add a completely different
switch.

Once you remove the requirement of a running server, we have basically
what has been recently implemented with postgres -C for
runtime-computed GUCs, because we already go through a read of the
control file to be able to print those GUCs with their correct
values.  This also means that it is already possible to check if a
data folder is compatible with a set of binaries with this facility,
as any postgres -C command with a runtime GUC would trigger this
check.  Using any of the existing runtime GUCs may be confusing, but
that would work.  And I am not really convinced that we have any need
to add a specific GUC for this purpose, be it a sort of
is_controlfile_valid or controlfile_checksum (CRC32 of the control
file).

>> Also, I think that we should be careful with the read of
>> the control file to avoid false positives?   We can rely on an atomic
>> read/write thanks to its maximum size of 512 bytes, but this looks
>> like a lot what has been done recently with postgres -C for runtime
>> GUCs, that *require* a read of the control file before grabbing the
>> values we are interested in.
>
> Sorry, I'm not following this one.  In my proposed patch, the control
> file (if one exists) is read after CreateDataDirLockFile(), just like
> PostmasterMain().

While looking at the patch, I was thinking about the fact that we may
want to support the case even if a server is running.  If we don't, my
worries about the concurrent control file activities are moot.
--
Michael

On Tue, Jan 25, 2022 at 01:12:32PM +0900, Michael Paquier wrote:
> Once you remove the requirement of a running server, we have basically
> what has been recently implemented with postgres -C for
> runtime-computed GUCs, because we already go through a read of the
> control file to be able to print those GUCs with their correct
> values.  This also means that it is already possible to check if a
> data folder is compatible with a set of binaries with this facility,
> as any postgres -C command with a runtime GUC would trigger this
> check.  Using any of the existing runtime GUCs may be confusing, but
> that would work.  And I am not really convinced that we have any need
> to add a specific GUC for this purpose, be it a sort of
> is_controlfile_valid or controlfile_checksum (CRC32 of the control
> file).

Thinking more about this one, we can already do that, so I have
marked the patch as RwF.  Perhaps we could just add a GUC, but that
feels a bit dummy.
--
Michael

On Mon, Jan 31, 2022 at 04:57:13PM +0900, Michael Paquier wrote:
> On Tue, Jan 25, 2022 at 01:12:32PM +0900, Michael Paquier wrote:
>> Once you remove the requirement of a running server, we have basically
>> what has been recently implemented with postgres -C for
>> runtime-computed GUCs, because we already go through a read of the
>> control file to be able to print those GUCs with their correct
>> values.  This also means that it is already possible to check if a
>> data folder is compatible with a set of binaries with this facility,
>> as any postgres -C command with a runtime GUC would trigger this
>> check.  Using any of the existing runtime GUCs may be confusing, but
>> that would work.  And I am not really convinced that we have any need
>> to add a specific GUC for this purpose, be it a sort of
>> is_controlfile_valid or controlfile_checksum (CRC32 of the control
>> file).
> 
> Thinking more about this one, we can already do that, so I have
> marked the patch as RwF.  Perhaps we could just add a GUC, but that
> feels a bit dummy.

Sorry, I missed this thread earlier.  You're right, we can just do
something like the following to achieve basically the same result:

    postgres -D . -C data_checksums

Unless Vik has any objections, this can probably be marked as Withdrawn.
Perhaps we can look into providing a new option for "postgres" at some
point in the future, but I don't sense a ton of demand at the moment.

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