Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)

[John Naylor]

On Sat, Jul 29, 2023 at 7:37 PM Ranier Vilela  wrote:
>
> Hi,
>
> The pg_leftmost_one_pos32 function with msvc compiler,
> relies on Assert to guarantee the correct result.
>
> But msvc documentation [1] says clearly that
> undefined behavior can occur.

It seems that we should have "Assert(word != 0);" at the top, which matches
the other platforms anyway, so I'll add that.

> Fix by checking the result of the function to avoid
> a possible undefined behavior.

No other platform does this, and that is by design. I'm also not
particularly impressed by the unrelated cosmetic changes.

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

Re: UUID v7

[Andrey M. Borodin]

On 10 Jul 2023, at 21:50, Peter Eisentraut  wrote:I suggest we keep this thread to v7, which has pretty straightforward semantics for PostgreSQL.  v8 by definition has many possible implementations, so you're going to have to make pretty strong arguments that yours is the best and only one, if you are going to claim the gen_uuid_v8 function name.Thanks Peter, I'll follow this course of action.After discussion on GitHub with Sergey Prokhorenko [0] I understood that counter is optional, but useful part of UUID v7. It actually promotes sortability of data generated at high speed.The standard does not specify how big counter should be. PFA patch with 16 bit counter. Maybe it worth doing 18bit counter - it will save us one byte of PRNG data. Currently we only take 2 bits out of the whole random byte.Best regards, Andrey Borodin.[0] https://github.com/x4m/pg_uuid_next/issues/1#issuecomment-1657074776

v3-0001-Implement-UUID-v7-as-per-IETF-draft.patch
Description: Binary data

Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)

[Tom Lane]

John Naylor  writes:
> It seems that we should have "Assert(word != 0);" at the top, which matches
> the other platforms anyway, so I'll add that.

That's basically equivalent to the existing Assert(non_zero).
I think it'd be okay to drop that one and instead have
the same Assert condition as other platforms, but having both
would be redundant.

I agree that adding the non-Assert test that Ranier wants
is entirely pointless.  If a caller did manage to violate
the asserted-for condition (and we don't have asserts on),
returning zero is not better than returning an unspecified
value.  If anything it might be worse, since it might not
lead to obvious misbehavior.

On the whole, there's not anything wrong with the code as-is.
A case could be made for making the MSVC asserts more like
other platforms, but it's quite cosmetic.

regards, tom lane

Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"

[Anton A. Melnikov]

Hello!

On 26.07.2023 07:06, Thomas Munro wrote:

 New patches
attached.  Are they getting better?


It seems to me that it is worth focusing efforts on the second part of the 
patch,
as the most in demand. And try to commit it first.

And seems there is a way to simplify it by adding a parameter to 
get_controlfile() that will return calculated
crc and moving the repetition logic level up.

There is a proposed algorithm in alg_level_up.pdf attached.

[Excuse me, for at least three days i will be in a place where there is no 
available Internet. \
So will be able to read this thread no earlier than August 2 evening]

With the best wishes,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

alg_level_up.pdf
Description: Adobe PDF document

Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"

[Anton A. Melnikov]

Sorry, attached the wrong version of the file. Here is the right one.

Sincerely yours,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

alg_level_up.pdf
Description: Adobe PDF document

Re: pg_rewind fails with in-place tablespace

[Michael Paquier]

On Fri, Jul 28, 2023 at 04:54:56PM +0900, Michael Paquier wrote:
> I am finishing with the attached.  Thoughts?

Applied this one as bf22792 on HEAD, without a backpatch as in-place
tablespaces are around for developers.  If there are opinions in favor
of a backpatch, feel free of course.
--
Michael


signature.asc
Description: PGP signature

Re: Support to define custom wait events for extensions

[Michael Paquier]

On Fri, Jul 28, 2023 at 12:43:36PM +0530, Bharath Rupireddy wrote:
> 1.
> - so an LWLock wait event might be reported as
> - just extension rather than the
> - extension-assigned name.
> + if the extension's library is not loaded; so a custom wait event might
> + be reported as just ???
> + rather than the custom name assigned.
> 
> Trying to understand why '???' is any better than 'extension' for a
> registered custom wait event of an unloaded extension?
> 
> PS: Looked at other instances where '???' is being used for
> representing an unknown "thing".

You are right that I am making things inconsistent here.  Having a
behavior close to the existing LWLock and use "extension" when the
event cannot be found makes the most sense.  I have been a bit
confused with the wording though of this part of the docs, though, as
LWLocks don't directly use the custom wait event APIs.

> 2. Have an example of how a custom wait event is displayed in the
> example in the docs "Here is an example of how wait events can be
> viewed:". We can use the worker_spi wait event type there.

Fine by me, added one.

> 3.
> - so an LWLock wait event might be reported as
> - just extension rather than the
> - extension-assigned name.
> 
> + . In some cases, the name
> + assigned by an extension will not be available in all server processes
> + if the extension's library is not loaded; so a custom wait event might
> + be reported as just ???
> 
> Are we missing to explicitly say what wait event will be reported for
> an LWLock when the extension library is not loaded?

Yes, see answer to point 1.

> 4.
> + Add-ins can define custom wait events under the wait event type
> 
> I see a few instances of Add-ins/add-in in xfunc.sgml. Isn't it better
> to use the word extension given that glossary defines what an
> extension is 
> https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-EXTENSION?

An extension is an Add-in, and may be loaded, but it is possible to
have modules that just need to be LOAD'ed (with command line or just
shared_preload_libraries) so an add-in may not always be an extension.
I am not completely sure if add-ins is the best term, but it covers
both, and that's consistent with the existing docs.  Perhaps the same
area of the docs should be refreshed, but that looks like a separate
patch for me.  For now, I'd rather use a consistent term, and this one
sounds OK to me.

[1]: https://www.postgresql.org/docs/devel/extend-extensions.html.

> 5.
> +} WaitEventExtensionCounter;
> +
> +/* pointer to the shared memory */
> +static WaitEventExtensionCounter *waitEventExtensionCounter;
> 
> How about naming the structure variable as
> WaitEventExtensionCounterData and pointer as
> WaitEventExtensionCounter? This keeps all the static variable names
> consistent WaitEventExtensionNames, WaitEventExtensionNamesAllocated
> and WaitEventExtensionCounter.

Hmm, good point on consistency here, especially to use an upper-case
character for the first characters of waitEventExtensionCounter..
Err..  WaitEventExtensionCounter.

> 6.
> +/* Check the wait event class. */
> +Assert((wait_event_info & 0xFF00) == PG_WAIT_EXTENSION);
> +
> +/* This should only be called for user-defined wait event. */
> +Assert(eventId >= NUM_BUILTIN_WAIT_EVENT_EXTENSION);
> 
> Maybe, we must turn the above asserts into ereport(ERROR) to protect
> against an extension sending in an unregistered wait_event_info?
> Especially, the first Assert((wait_event_info & 0xFF00) ==
> PG_WAIT_EXTENSION); checks that the passed in wait_event_info is
> previously returned by WaitEventExtensionNew. IMO, these assertions
> better fit for errors.

Okay by me that it may be a better idea to use ereport(ERROR) in the
long run, so changed this way.  I have introduced a
WAIT_EVENT_CLASS_MASK and a WAIT_EVENT_ID_MASK as we now use
0xFF00 and 0x in three places of this file.  This should
just be a patch on its own.

> 7.
> + * Extensions can define their own wait events in this categiry.  First,
> Typo - s/categiry/category

Thanks, missed that.

> 8.
> + First,
> + * they should call WaitEventExtensionNew() to get one or more wait event
> + * IDs that are allocated from a shared counter.
> 
> Can WaitEventExtensionNew() be WaitEventExtensionNew(int num_ids, int
> *result) to get the required number of wait event IDs in one call
> similar to RequestNamedLWLockTranche? Currently, an extension needs to
> call WaitEventExtensionNew() N number of times to get N wait event
> IDs. Maybe the existing WaitEventExtensionNew() is good, but just a
> thought.

Yes, this was mentioned upthread.  I am not completely sure yet how
much we need to do for this interface, but surely it would be faster
to have a Multiple() interface that returns an array made of N numbers
requested (rather than a rank of them).  For now, I'd rather just aim
for simplicity for the basics.

> 9.
> # The expected result is a spe

Re: New PostgreSQL Contributors

[Dianjin Wang]

Bingo work! Thanks to all the contributors!

Best,
Dianjin Wang


On Fri, Jul 28, 2023 at 11:29 PM Christoph Berg  wrote:

> The PostgreSQL contributors team has been looking over the community
> activity and, over the first half of this year, has been recognizing
> new contributors to be listed on
>
> https://www.postgresql.org/community/contributors/
>
> New PostgreSQL Contributors:
>
>   Ajin Cherian
>   Alexander Kukushkin
>   Alexander Lakhin
>   Dmitry Dolgov
>   Hou Zhijie
>   Ilya Kosmodemiansky
>   Melanie Plageman
>   Michael Banck
>   Michael Brewer
>   Paul Jungwirth
>   Peter Smith
>   Vigneshwaran C
>
> New PostgreSQL Major Contributors:
>
>   Julien Rouhaud
>   Stacey Haysler
>   Steve Singer
>
> Thank you all for contributing to PostgreSQL to make it such a great
> project!
>
> For the contributors team,
> Christoph
>
>
>

Adding a LogicalRepWorker type field

[Peter Smith]

Hi hackers,

BACKGROUND:

The logical replication has different worker "types" (e.g. apply
leader, apply parallel, tablesync).

They all use a common structure called LogicalRepWorker, but at times
it is necessary to know what "type" of worker a given LogicalRepWorker
represents.

Once, there were just apply workers and tablesync workers - these were
easily distinguished because only tablesync workers had a valid
'relid' field. Next, parallel-apply workers were introduced - these
are distinguishable from the apply leaders by the value of
'leader_pid' field.


PROBLEM:

IMO, deducing the worker's type by examining multiple different field
values seems a dubious way to do it. This maybe was reasonable enough
when there were only 2 types, but as more get added it becomes
increasingly complicated.

SOLUTION:

AFAIK none of the complications is necessary anyway; the worker type
is already known at launch time, and it never changes during the life
of the process.

The attached Patch 0001 introduces a new enum 'type' field, which is
assigned during the launch.

~

This change not only simplifies the code, but it also permits other
code optimizations, because now we can switch on the worker enum type,
instead of using cascading if/else.  (see Patch 0002).

Thoughts?

--
Kind Regards,
Peter Smith.
Fujitsu Australia


v1-0001-Add-LogicalRepWorkerType-enum.patch
Description: Binary data


v1-0002-Switch-on-worker-type.patch
Description: Binary data

Re: 回复：pg_rewind fails with in-place tablespace

[Michael Paquier]

On Mon, Jul 31, 2023 at 10:07:44AM +0800, Rui Zhao wrote:
> However, I would like to bring your attention to another issue:
> pg_upgrade fails with in-place tablespace. Another issue is still
> waiting for approved. I have tested all the tools in src/bin with
> in-place tablespace, and I believe this is the final issue. 

No problem.  Please feel free to start a new thread about that, I'm
okay to look at what you would like to propose.  Adding a test in
002_pg_upgrade.pl where the pg_upgrade runs happen would be a good
thing to have, I guess.
--
Michael


signature.asc
Description: PGP signature

Re: New PostgreSQL Contributors

[Chris Travers]

Congrats!

On Fri, Jul 28, 2023 at 10:29 PM Christoph Berg  wrote:

> The PostgreSQL contributors team has been looking over the community
> activity and, over the first half of this year, has been recognizing
> new contributors to be listed on
>
> https://www.postgresql.org/community/contributors/
>
> New PostgreSQL Contributors:
>
>   Ajin Cherian
>   Alexander Kukushkin
>   Alexander Lakhin
>   Dmitry Dolgov
>   Hou Zhijie
>   Ilya Kosmodemiansky
>   Melanie Plageman
>   Michael Banck
>   Michael Brewer
>   Paul Jungwirth
>   Peter Smith
>   Vigneshwaran C
>
> New PostgreSQL Major Contributors:
>
>   Julien Rouhaud
>   Stacey Haysler
>   Steve Singer
>
> Thank you all for contributing to PostgreSQL to make it such a great
> project!
>
> For the contributors team,
> Christoph
>
>
>

Re: New PostgreSQL Contributors

[Amul Sul]

On Fri, Jul 28, 2023 at 8:59 PM Christoph Berg  wrote:

> The PostgreSQL contributors team has been looking over the community
> activity and, over the first half of this year, has been recognizing
> new contributors to be listed on
>
> https://www.postgresql.org/community/contributors/
>
> New PostgreSQL Contributors:
>
>   Ajin Cherian
>   Alexander Kukushkin
>   Alexander Lakhin
>   Dmitry Dolgov
>   Hou Zhijie
>   Ilya Kosmodemiansky
>   Melanie Plageman
>   Michael Banck
>   Michael Brewer
>   Paul Jungwirth
>   Peter Smith
>   Vigneshwaran C
>
> New PostgreSQL Major Contributors:
>
>   Julien Rouhaud
>   Stacey Haysler
>   Steve Singer
>

Many congratulations !!

Regards,
Amul

Re: New PostgreSQL Contributors

[Rahila Syed]

Hi,

On Fri, Jul 28, 2023 at 8:59 PM Christoph Berg  wrote:

> The PostgreSQL contributors team has been looking over the community
> activity and, over the first half of this year, has been recognizing
> new contributors to be listed on
>
> https://www.postgresql.org/community/contributors/
>
> New PostgreSQL Contributors:
>
>   Ajin Cherian
>   Alexander Kukushkin
>   Alexander Lakhin
>   Dmitry Dolgov
>   Hou Zhijie
>   Ilya Kosmodemiansky
>   Melanie Plageman
>   Michael Banck
>   Michael Brewer
>   Paul Jungwirth
>   Peter Smith
>   Vigneshwaran C
>
> New PostgreSQL Major Contributors:
>
>   Julien Rouhaud
>   Stacey Haysler
>   Steve Singer
>
> Congratulations to all the new contributors!

Thank you,
Rahila syed

Re: add timing information to pg_upgrade

[Bharath Rupireddy]

On Sun, Jul 30, 2023 at 2:44 AM Nathan Bossart  wrote:
>
> On Sat, Jul 29, 2023 at 12:17:40PM +0530, Bharath Rupireddy wrote:
> > While on this, I noticed a thing unrelated to your patch that there's
> > no space between the longest status message of size 60 bytes and ok -
> > 'Checking for incompatible "aclitem" data type in user tablesok
> > 23.932 ms'. I think MESSAGE_WIDTH needs to be bumped up - 64 or more.
>
> Good catch.  I think I'd actually propose just removing "in user tables" or
> the word "incompatible" from these messages to keep them succinct.

Either of "Checking for \"aclitem\" data type usage" or "Checking for
\"aclitem\" data type in user tables"  seems okay to me, however, I
prefer the second wording.

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

Re: Support to define custom wait events for extensions

[Bharath Rupireddy]

On Mon, Jul 31, 2023 at 6:40 AM Michael Paquier  wrote:
>
> You are right that I am making things inconsistent here.  Having a
> behavior close to the existing LWLock and use "extension" when the
> event cannot be found makes the most sense.  I have been a bit
> confused with the wording though of this part of the docs, though, as
> LWLocks don't directly use the custom wait event APIs.

+ * calling WaitEventExtensionRegisterName() in the current process, in
+ * which case give up and return an unknown state.

We're not giving up and returning an unknown state in the v10 patch
unlike v9, no? This comment needs to change.

> > 4.
> > + Add-ins can define custom wait events under the wait event type
> >
> > I see a few instances of Add-ins/add-in in xfunc.sgml. Isn't it better
> > to use the word extension given that glossary defines what an
> > extension is 
> > https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-EXTENSION?
>
> An extension is an Add-in, and may be loaded, but it is possible to
> have modules that just need to be LOAD'ed (with command line or just
> shared_preload_libraries) so an add-in may not always be an extension.
> I am not completely sure if add-ins is the best term, but it covers
> both, and that's consistent with the existing docs.  Perhaps the same
> area of the docs should be refreshed, but that looks like a separate
> patch for me.  For now, I'd rather use a consistent term, and this one
> sounds OK to me.
>
> [1]: https://www.postgresql.org/docs/devel/extend-extensions.html.

The "external module" seems the right wording here. Use of "add-ins"
is fine by me for this patch.

> Okay by me that it may be a better idea to use ereport(ERROR) in the
> long run, so changed this way.  I have introduced a
> WAIT_EVENT_CLASS_MASK and a WAIT_EVENT_ID_MASK as we now use
> 0xFF00 and 0x in three places of this file.  This should
> just be a patch on its own.

Yeah, I don't mind these macros going along or before or after the
custom wait events feature.

> Yes, this was mentioned upthread.  I am not completely sure yet how
> much we need to do for this interface, but surely it would be faster
> to have a Multiple() interface that returns an array made of N numbers
> requested (rather than a rank of them).  For now, I'd rather just aim
> for simplicity for the basics.

+1 to be simple for now. If any such requests come in future, I'm sure
we can always get back to it.

> > 9.
> > # The expected result is a special pattern here with a newline coming from 
> > the
> > # first query where the shared memory state is set.
> > $result = $node->poll_query_until(
> > 'postgres',
> > qq[SELECT worker_spi_init(); SELECT wait_event FROM
> > pg_stat_activity WHERE backend_type ~ 'worker_spi';],
> > qq[
> > worker_spi_main]);
> >
> > This test doesn't have to be that complex with the result being a
> > special pattern, SELECT worker_spi_init(); can just be within a
> > separate safe_psql.
>
> No, it cannot because we need the custom wait event string to be
> loaded in the same connection as the one querying pg_stat_activity.
> A different thing that can be done here is to use background_psql()
> with a query_until(), though I am not sure that this is worth doing
> here.

-1 to go to the background_psql and query_until route. However,
enhancing the comment might help "we need the custom wait event string
to be loaded in the same connection as .". Having said this, I
don't quite understand the point of having worker_spi_init() when we
say clearly how to use shmem hooks and custom wait events. If its
intention is to show loading of shared memory to a backend via a
function, do we really need it in worker_spi? I don't mind removing it
if it's not adding any significant value.

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

Re: Support to define custom wait events for extensions

[Masahiro Ikeda]

On 2023-07-31 10:10, Michael Paquier wrote:

Attached is a new version.


Thanks for all the improvements.
I have some comments for v10.

(1)


 
- Extensions can add LWLock types to the list 
shown in
- .  In some cases, the 
name

+ Extensions can add Extension and
+ LWLock types
+ to the list shown in  
and

+ . In some cases, the name
  assigned by an extension will not be available in all server 
processes;

- so an LWLock wait event might be reported as
- just extension rather than the
+ so an LWLock or Extension 
wait

+ event might be reported as just
+ extension rather than the
  extension-assigned name.
 


I think the order in which they are mentioned should be matched. I mean 
that
- so an LWLock or Extension 
wait
+ so an Extension or LWLock 
wait



(2)

/* This should only be called for user-defined wait event. */
if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid wait event ID %u", eventId));

I was just wondering if it should also check the eventId
that has been allocated though it needs to take the spinlock
and GetWaitEventExtensionIdentifier() doesn't take it into account.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Re: Support to define custom wait events for extensions

[Michael Paquier]

On Mon, Jul 31, 2023 at 12:07:40PM +0530, Bharath Rupireddy wrote:
> We're not giving up and returning an unknown state in the v10 patch
> unlike v9, no? This comment needs to change.

Right.  Better to be consistent with lwlock.c here.

>> No, it cannot because we need the custom wait event string to be
>> loaded in the same connection as the one querying pg_stat_activity.
>> A different thing that can be done here is to use background_psql()
>> with a query_until(), though I am not sure that this is worth doing
>> here.
> 
> -1 to go to the background_psql and query_until route. However,
> enhancing the comment might help "we need the custom wait event string
> to be loaded in the same connection as .". Having said this, I
> don't quite understand the point of having worker_spi_init() when we
> say clearly how to use shmem hooks and custom wait events. If its
> intention is to show loading of shared memory to a backend via a
> function, do we really need it in worker_spi? I don't mind removing it
> if it's not adding any significant value.

It seems to initialize the state of the worker_spi, so attaching a
function to this stuff makes sense to me, just for the sake of testing
all that.
--
Michael


signature.asc
Description: PGP signature