Re: On login trigger: take three

[Ted Yu]

On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov  wrote:

> Hi Nikita,
>
> > Mikhail, I've checked the patch and previous discussion,
> > the condition mentioned earlier is still actual:
>
> Thanks for pointing this out! My bad, I forgot to fix the documentation
> example.
> Attached v34 has this issue fixed, as well as a couple other problems with
> the example code.
>
> --
>  best regards,
> Mikhail A. Gribkov
>
Hi,

bq. in to the system

'in to' -> into

bq. Any bugs in a trigger procedure

Any bugs -> Any bug

+   if (event == EVT_Login)
+   dbgtag = CMDTAG_LOGIN;
+   else
+   dbgtag = CreateCommandTag(parsetree);

The same snippet appears more than once. It seems CMDTAG_LOGIN handling can
be moved into `CreateCommandTag`.

Cheers

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

[Ted Yu]

On Fri, Dec 16, 2022 at 10:04 PM Nathan Bossart 
wrote:

> On Thu, Dec 15, 2022 at 10:10:43AM -0800, Jeff Davis wrote:
> > The proposal to skip privilege checks for partitions would be
> > consistent with INSERT, SELECT, REINDEX that flow through to the
> > underlying partitions regardless of permissions/ownership (and even
> > RLS). It would be very minor behavior change on 15 for this weird case
> > of superuser-owned partitions, but I doubt anyone would be relying on
> > that.
>
> I've attached a work-in-progress patch that aims to accomplish this.
> Instead of skipping the privilege checks, I added logic to trawl through
> pg_inherits and pg_class to check whether the user has privileges for the
> partitioned table or for the main relation of a TOAST table.  This means
> that MAINTAIN on a partitioned table is enough to execute maintenance
> commands on all the partitions, and MAINTAIN on a main relation is enough
> to execute maintenance commands on its TOAST table.  Also, the maintenance
> commands that flow through to the partitions or the TOAST table should no
> longer error due to permissions when the user only has MAINTAIN on the
> paritioned table or main relation.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com

Hi,

+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+   return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
ACLCHECK_OK ||
+  has_parent_privs(relid, userid, ACL_MAINTAIN);

Since the func only contains one statement, it seems this can be defined as
a macro instead.

+   List   *ancestors = get_partition_ancestors(relid);
+   Oid root = InvalidOid;

nit: it would be better if the variable `root` can be aligned with variable
`ancestors`.

Cheers

Re: Rework of collation code, extensibility

[Ted Yu]

On Sat, Dec 17, 2022 at 7:14 PM Jeff Davis  wrote:

> Attached is a new patch series. I think there are enough changes that
> this has become more of a "rework" of the collation code rather than
> just a refactoring. This is a continuation of some prior work[1][2] in
> a new thread given its new scope.
>
> Benefits:
>
> 1. Clearer division of responsibilities.
> 2. More consistent between libc and ICU providers.
> 3. Hooks that allow extensions to replace collation provider libraries.
> 4. New tests for the collation provider library hooks.
>
> There are a lot of changes, and still some loose ends, but I believe a
> few of these patches are close to ready.
>
> This set of changes does not express an opinion on how we might want to
> support multiple provider libraries in core; but whatever we choose, it
> should be easier to accomplish. Right now, the hooks have limited
> information on which to make the choice for a specific version of a
> collation provider library, but that's because there's limited
> information in the catalog. If the discussion here[3] concludes in
> adding collation provider library or library version information to the
> catalog, we can add additional parameters to the hooks.
>
> [1]
>
> https://postgr.es/m/99aa79cceefd1fe84fda23510494b8fbb7ad1e70.ca...@j-davis.com
> [2]
>
> https://postgr.es/m/c4fda90ec6a7568a896f243a38eb273c3b5c3d93.ca...@j-davis.com
> [3]
>
> https://postgr.es/m/ca+hukgleqmhnpzrgacisoueyfgz8w6ewdhtk2h-4qn0iosf...@mail.gmail.com
>
>
> --
> Jeff Davis
> PostgreSQL Contributor Team - AWS
>
> Hi,
For pg_strxfrm_libc in v4-0002-Add-pg_strxfrm-and-pg_strxfrm_prefix.patch:

+#ifdef HAVE_LOCALE_T
+   if (locale)
+   return strxfrm_l(dest, src, destsize, locale->info.lt);
+   else
+#endif
+   return strxfrm(dest, src, destsize);

It seems the `else` is not needed (since when the if branch is taken, we
return from the func).

+   /* nul-terminate arguments */

nul-terminate -> null-terminate

For pg_strnxfrm(), I think `result` can be removed - we directly return the
result from pg_strnxfrm_libc or pg_strnxfrm_icu.

Cheers

Re: Rework of collation code, extensibility

[Ted Yu]

On Sat, Dec 17, 2022 at 8:54 PM John Naylor 
wrote:

>
> > nul-terminate -> null-terminate
>
> NUL is a common abbreviation for the zero byte (but not for zero
> pointers). See the ascii manpage.
>
> --
> John Naylor
> EDB: http://www.enterprisedb.com
>
> Ah.

`nul-terminated` does appear in the codebase.
Should have checked earlier.

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

[Ted Yu]

On Sun, Dec 18, 2022 at 3:30 PM Nathan Bossart 
wrote:

> Here is a new version of the patch.  Besides some cleanup, I added an index
> on reltoastrelid for the toast-to-main-relation lookup.  Before I bother
> adjusting the tests and documentation, I'm curious to hear thoughts on
> whether this seems like a viable approach.
>
> On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> > +cluster_is_permitted_for_relation(Oid relid, Oid userid)
> > +{
> > +   return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
> > ACLCHECK_OK ||
> > +  has_parent_privs(relid, userid, ACL_MAINTAIN);
> >
> > Since the func only contains one statement, it seems this can be defined
> as
> > a macro instead.
>
> In the new version, there is a bit more to this function, so I didn't
> convert it to a macro.
>
> > +   List   *ancestors = get_partition_ancestors(relid);
> > +   Oid root = InvalidOid;
> >
> > nit: it would be better if the variable `root` can be aligned with
> variable
> > `ancestors`.
>
> Hm.  It looked alright on my machine.  In any case, I'll be sure to run
> pgindent at some point.  This patch is still in early stages.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com

Hi,

+ * Note: Because this function assumes that the realtion whose OID is
passed as

Typo:  realtion -> relation

Cheers

Re: On login trigger: take three

[Ted Yu]

On Mon, Dec 19, 2022 at 1:40 AM Mikhail Gribkov  wrote:

> Hi Ted,
>
> > bq. in to the system
> > 'in to' -> into
> > bq. Any bugs in a trigger procedure
> > Any bugs -> Any bug
>
> Thanks!  Fixed typos in the attached v35.
>
> >   +   if (event == EVT_Login)
> >   +   dbgtag = CMDTAG_LOGIN;
> >   +   else
> >   +   dbgtag = CreateCommandTag(parsetree);
> > The same snippet appears more than once. It seems CMDTAG_LOGIN handling
> can be moved into `CreateCommandTag`.
>
> It appears twice in fact, both cases are nearby: in the main code and
> under the assert-checking ifdef.
> Moving CMDTAG_LOGIN to CreateCommandTag would change the CreateCommandTag
> function signature and ideology. CreateCommandTag finds a tag based on the
> command parse tree, while login event is a specific case when we do not
> have any command and the parse tree is NULL. Changing CreateCommandTag
> signature for these two calls looks a little bit overkill as it would lead
> to changing lots of other places the function is called from.
> We could move this snippet to a separate function, but here are the
> similar concerns I think: it would make sense for a more common or a more
> complex snippet, but not for two cases of if-else one-liners.
>
> --
>  best regards,
> Mikhail A. Gribkov
>
> e-mail: youzh...@gmail.com
> *http://www.flickr.com/photos/youzhick/albums
> *
> http://www.strava.com/athletes/5085772
> phone: +7(916)604-71-12
> Telegram: @youzhick
>
> Hi, Mikhail:
Thanks for the explanation.
It is Okay to keep the current formation of CMDTAG_LOGIN handling.

Cheers

Fixing typo in 002_pg_dump.pl

[Ted Yu]

Hi,
I was going over 002_pg_dump.pl and saw a typo in pgdump_runs.

Please see the patch.

Thanks


pg-dump-comment.patch
Description: Binary data

Re: Add sub-transaction overflow status in pg_stat_activity

[Ted Yu]

On Mon, Dec 19, 2022 at 11:57 AM Robert Haas  wrote:

> On Tue, Dec 13, 2022 at 2:29 AM Julien Rouhaud  wrote:
> > > > Makes sense.
> > >
> > > +1
> >
> > +1
>
> Committed with a bit more word-smithing on the documentation.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>
> Hi,
It seems the comment for `backend_subxact_overflowed` missed a word.

Please see the patch.

Thanks


subtxn-number-comment.patch
Description: Binary data

Re: checking snapshot argument for index_beginscan

[Ted Yu]

>
> Hi,
> I was looking at commit 941aa6a6268a6a66f6895401aad6b5329111d412 .
>
> I think it would be better to move the assertion into
> `index_beginscan_internal` because it is called by index_beginscan
> and index_beginscan_bitmap
>
> In the future, when a new variant of `index_beginscan_XX` is added, the
> assertion would be effective for the new variant.
>
> Please let me know what you think.
>
> Cheers
>


index-begin-scan-check-snapshot.patch
Description: Binary data

Re: Making Vars outer-join aware

[Ted Yu]

On Fri, Dec 23, 2022 at 10:21 AM Tom Lane  wrote:

> Here's a new edition of this patch series.
>
> I shoved some preliminary refactoring into the 0001 patch,
> notably splitting deconstruct_jointree into two passes.
> 0002-0009 cover the same ground as they did before, though
> with some differences in detail.  0010-0012 are new work
> mostly aimed at removing kluges we no longer need.
>
> There are two big areas that I would still like to improve, but
> I think we've run out of time to address them in the v16 cycle:
>
> * It'd be nice to apply the regular EquivalenceClass deduction
> mechanisms to outer-join equalities, instead of the
> reconsider_outer_join_clauses kluge.  I've made several stabs at that
> without much success.  I think that the "join domain" framework added
> in 0012 is likely to provide a workable foundation, but some more
> effort is needed.
>
> * I really want to get rid of RestrictInfo.is_pushed_down and
> RINFO_IS_PUSHED_DOWN(), because those seem exceedingly awkward
> and squishy.  I've not gotten far with finding a better
> replacement there, either.
>
> Despite the work being unfinished, I feel that this has moved us a
> long way towards outer-join handling being less of a jury-rigged
> affair.
>
> regards, tom lane
>
> Hi,
For v8-0012-invent-join-domains.patch, in `distribute_qual_to_rels`, it
seems that `pseudoconstant` and `root->hasPseudoConstantQuals` carry the
same value.
Can the variable `pseudoconstant` be omitted ?

Cheers

Re: Error-safe user functions

[Ted Yu]

On Fri, Dec 23, 2022 at 9:20 AM Andrew Dunstan  wrote:

>
> On 2022-12-22 Th 11:44, Tom Lane wrote:
> > Andrew Dunstan  writes:
> >> Yeah, I started there, but it's substantially more complex - unlike cube
> >> the jsonpath scanner calls the error routines as well as the parser.
> >> Anyway, here's a patch.
> > I looked through this and it seems generally OK.  A minor nitpick is
> > that we usually write "(Datum) 0" not "(Datum) NULL" for dont-care Datum
> > values.
>
>
> Fixed in the new version attached.
>
>
> > A slightly bigger issue is that makeItemLikeRegex still allows
> > an error to be thrown from RE_compile_and_cache if a bogus regex is
> > presented.  But that could be dealt with later.
>
>
> I'd rather fix it now while we're paying attention.
>
>
> >
> > (I wonder why this is using RE_compile_and_cache at all, really,
> > rather than some other API.  There doesn't seem to be value in
> > forcing the regex into the cache at this point.)
> >
> >
>
>
> I agree. The attached uses pg_regcomp instead. I had a lift a couple of
> lines from regexp.c, but not too many.
>
>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

Hi,
In makeItemLikeRegex :

+   /* See regexp.c for explanation */
+   CHECK_FOR_INTERRUPTS();
+   pg_regerror(re_result, &re_tmp, errMsg,
sizeof(errMsg));
+   ereturn(escontext, false,

Since an error is returned, I wonder if the `CHECK_FOR_INTERRUPTS` call is
still necessary.

 Cheers

Re: Error-safe user functions

[Ted Yu]

On Fri, Dec 23, 2022 at 1:22 PM Tom Lane  wrote:

> Ted Yu  writes:
> > In makeItemLikeRegex :
>
> > +   /* See regexp.c for explanation */
> > +   CHECK_FOR_INTERRUPTS();
> > +   pg_regerror(re_result, &re_tmp, errMsg,
> > sizeof(errMsg));
> > +   ereturn(escontext, false,
>
> > Since an error is returned, I wonder if the `CHECK_FOR_INTERRUPTS` call
> is
> > still necessary.
>
> Yes, it is.  We don't want a query-cancel transformed into a soft error.
>
> regards, tom lane
>
Hi,
`ereturn(escontext` calls appear in multiple places in the patch.
What about other callsites (w.r.t. checking interrupt) ?

Cheers

Re: Error-safe user functions

[Ted Yu]

On Fri, Dec 23, 2022 at 1:22 PM Tom Lane  wrote:

> Ted Yu  writes:
> > In makeItemLikeRegex :
>
> > +   /* See regexp.c for explanation */
> > +   CHECK_FOR_INTERRUPTS();
> > +   pg_regerror(re_result, &re_tmp, errMsg,
> > sizeof(errMsg));
> > +   ereturn(escontext, false,
>
> > Since an error is returned, I wonder if the `CHECK_FOR_INTERRUPTS` call
> is
> > still necessary.
>
> Yes, it is.  We don't want a query-cancel transformed into a soft error.
>
> regards, tom lane
>
Hi,
For this case (`invalid regular expression`), the potential user
interruption is one reason for stopping execution.
I feel surfacing user interruption somehow masks the underlying error.

The same regex, without user interruption, would exhibit an `invalid
regular expression` error.
I think it would be better to surface the error.

Cheers

Re: Error-safe user functions

[Ted Yu]

On Sat, Dec 24, 2022 at 4:38 AM Andrew Dunstan  wrote:

>
> On 2022-12-24 Sa 04:51, Ted Yu wrote:
> >
> >
> > On Fri, Dec 23, 2022 at 1:22 PM Tom Lane  wrote:
> >
> > Ted Yu  writes:
> > > In makeItemLikeRegex :
> >
> > > +   /* See regexp.c for explanation */
> > > +   CHECK_FOR_INTERRUPTS();
> > > +   pg_regerror(re_result, &re_tmp, errMsg,
> > > sizeof(errMsg));
> > > +   ereturn(escontext, false,
> >
> > > Since an error is returned, I wonder if the
> > `CHECK_FOR_INTERRUPTS` call is
> > > still necessary.
> >
> > Yes, it is.  We don't want a query-cancel transformed into a soft
> > error.
> >
> > regards, tom lane
> >
> > Hi,
> > For this case (`invalid regular expression`), the potential user
> > interruption is one reason for stopping execution.
> > I feel surfacing user interruption somehow masks the underlying error.
> >
> > The same regex, without user interruption, would exhibit an `invalid
> > regular expression` error.
> > I think it would be better to surface the error.
> >
> >
>
> All that this patch is doing is replacing a call to
> RE_compile_and_cache, which calls CHECK_FOR_INTERRUPTS, with similar
> code, which gives us the opportunity to call ereturn instead of ereport.
> Note that where escontext is NULL (the common case), ereturn functions
> identically to ereport. So unless you want to argue that the logic in
> RE_compile_and_cache is wrong I don't see what we're arguing about. If
> instead I had altered the API of RE_compile_and_cache to include an
> escontext parameter we wouldn't be having this argument at all. The only
> reason I didn't do that was the point Tom quite properly raised about
> why we're doing any caching here anyway.
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

Andrew:

Thanks for the response.

Re: [Proposal] Add foreign-server health checks infrastructure

[Ted Yu]

On Tue, Jan 10, 2023 at 8:26 AM Hayato Kuroda (Fujitsu) <
kuroda.hay...@fujitsu.com> wrote:

> Dear tom,
>
> > I think that it's a really bad idea to require postgres_fdw.sql
> > to have two expected-files: that will be a maintenance nightmare.
> > Please put whatever it is that needs a variant expected-file
> > into its own, hopefully very small and seldom-changed, test script.
> > Or rethink whether you really need a test case that has
> > platform-dependent output.
>
> Thank you for giving the suggestion. I agreed your saying and modifed that.
>
> I added new functions on the libpq and postgres-fdw layer that check
> whether the
> checking works well or not. In the test, at first, the platform is checked
> and
> the checking function is called only when it is supported.
>
> An alternative approach is that PQCanConncheck() can be combined with
> PQConncheck().
> This can reduce the libpq function, but we must define another returned
> value to
> the function like -2. I was not sure which approach was better.
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
>

Hi,

+   /* quick exit if connection cache has been not initialized yet. */

been not initialized -> not been initialized

+
(errcode(ERRCODE_CONNECTION_FAILURE),
+errmsg("could not connect
to server \"%s\"",

Currently each server which is not connected would log a warning.
Is it better to concatenate names for such servers and log one line ? This
would be cleaner when there are multiple such servers.

Cheers

releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

[Ted Yu]

Hi,
I was reading src/backend/replication/logical/applyparallelworker.c .
In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
think the `ParallelApplyTxnHash` should be released.

Please see the patch.

Thanks

Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

[Ted Yu]

On Tue, Jan 10, 2023 at 9:25 AM Ted Yu  wrote:

> Hi,
> I was reading src/backend/replication/logical/applyparallelworker.c .
> In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
> think the `ParallelApplyTxnHash` should be released.
>
> Please see the patch.
>
> Thanks
>
Here is the patch :-)


destroy-parallel-apply-txn-hash-when-worker-not-launched.patch
Description: Binary data

Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

[Ted Yu]

On Tue, Jan 10, 2023 at 9:26 AM Ted Yu  wrote:

>
>
> On Tue, Jan 10, 2023 at 9:25 AM Ted Yu  wrote:
>
>> Hi,
>> I was reading src/backend/replication/logical/applyparallelworker.c .
>> In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
>> think the `ParallelApplyTxnHash` should be released.
>>
>> Please see the patch.
>>
>> Thanks
>>
> Here is the patch :-)
>

In `pa_process_spooled_messages_if_required`, the `pa_unlock_stream` call
immediately follows `pa_lock_stream`.
I assume the following is the intended sequence of calls. If this is the
case, I can add it to the patch.

Cheers

diff --git a/src/backend/replication/logical/applyparallelworker.c
b/src/backend/replication/logical/applyparallelworker.c
index 2e5914d5d9..9879b3fff2 100644
--- a/src/backend/replication/logical/applyparallelworker.c
+++ b/src/backend/replication/logical/applyparallelworker.c
@@ -684,9 +684,9 @@ pa_process_spooled_messages_if_required(void)
 if (fileset_state == FS_SERIALIZE_IN_PROGRESS)
 {
 pa_lock_stream(MyParallelShared->xid, AccessShareLock);
-pa_unlock_stream(MyParallelShared->xid, AccessShareLock);

 fileset_state = pa_get_fileset_state();
+pa_unlock_stream(MyParallelShared->xid, AccessShareLock);
 }

 /*

Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

[Ted Yu]

On Tue, Jan 10, 2023 at 9:43 AM Ted Yu  wrote:

>
>
> On Tue, Jan 10, 2023 at 9:26 AM Ted Yu  wrote:
>
>>
>>
>> On Tue, Jan 10, 2023 at 9:25 AM Ted Yu  wrote:
>>
>>> Hi,
>>> I was reading src/backend/replication/logical/applyparallelworker.c .
>>> In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
>>> think the `ParallelApplyTxnHash` should be released.
>>>
>>> Please see the patch.
>>>
>>> Thanks
>>>
>> Here is the patch :-)
>>
>
> In `pa_process_spooled_messages_if_required`, the `pa_unlock_stream` call
> immediately follows `pa_lock_stream`.
> I assume the following is the intended sequence of calls. If this is the
> case, I can add it to the patch.
>
> Cheers
>
> diff --git a/src/backend/replication/logical/applyparallelworker.c
> b/src/backend/replication/logical/applyparallelworker.c
> index 2e5914d5d9..9879b3fff2 100644
> --- a/src/backend/replication/logical/applyparallelworker.c
> +++ b/src/backend/replication/logical/applyparallelworker.c
> @@ -684,9 +684,9 @@ pa_process_spooled_messages_if_required(void)
>  if (fileset_state == FS_SERIALIZE_IN_PROGRESS)
>  {
>  pa_lock_stream(MyParallelShared->xid, AccessShareLock);
> -pa_unlock_stream(MyParallelShared->xid, AccessShareLock);
>
>  fileset_state = pa_get_fileset_state();
> +pa_unlock_stream(MyParallelShared->xid, AccessShareLock);
>  }
>
>  /*
>
Looking closer at the comment above this code and other part of the file,
it seems the order is intentional.

Please disregard my email about `pa_process_spooled_messages_if_required`.

Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

[Ted Yu]

On Tue, Jan 10, 2023 at 6:13 PM houzj.f...@fujitsu.com <
houzj.f...@fujitsu.com> wrote:

> On Wednesday, January 11, 2023 1:25 AM Ted Yu  wrote:
>
> > I was reading src/backend/replication/logical/applyparallelworker.c .
> > In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
> think the `ParallelApplyTxnHash` should be released.
>
> Thanks for reporting.
>
> ParallelApplyTxnHash is used to cache the state of streaming transactions
> being
> applied. There could be multiple streaming transactions being applied in
> parallel and their information were already saved in ParallelApplyTxnHash,
> so
> we should not release them just because we don't have a worker available to
> handle a new transaction here.
>
> Best Regards,
> Hou zj
>
Hi,

/* First time through, initialize parallel apply worker state
hashtable. */
if (!ParallelApplyTxnHash)

I think it would be better if `ParallelApplyTxnHash` is created by the
first successful parallel apply worker.

Please take a look at the new patch and see if it makes sense.

Cheers


create-parallel-apply-txn-hash-after-the-first-worker.patch
Description: Binary data

Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

[Ted Yu]

On Tue, Jan 10, 2023 at 7:55 PM houzj.f...@fujitsu.com <
houzj.f...@fujitsu.com> wrote:

> On Wednesday, January 11, 2023 10:21 AM Ted Yu 
> wrote:
> > /* First time through, initialize parallel apply worker state
> hashtable. */
> > if (!ParallelApplyTxnHash)
> >
> > I think it would be better if `ParallelApplyTxnHash` is created by the
> first
> > successful parallel apply worker.
>
> Thanks for the suggestion. But I am not sure if it's worth to changing the
> order here, because It will only optimize the case where user enable
> parallel
> apply but never get an available worker which should be rare. And in such a
> case, it'd be better to increase the number of workers or disable the
> parallel mode.
>
> Best Regards,
> Hou zj
>

I think even though the chance is rare, we shouldn't leak resource.

The `ParallelApplyTxnHash` shouldn't be created if there is no single apply
worker.

Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

[Ted Yu]

On Wed, Jan 11, 2023 at 6:54 PM Amit Kapila  wrote:

> On Wed, Jan 11, 2023 at 9:31 AM Ted Yu  wrote:
> >
> > On Tue, Jan 10, 2023 at 7:55 PM houzj.f...@fujitsu.com <
> houzj.f...@fujitsu.com> wrote:
> >>
> >> On Wednesday, January 11, 2023 10:21 AM Ted Yu 
> wrote:
> >> > /* First time through, initialize parallel apply worker state
> hashtable. */
> >> > if (!ParallelApplyTxnHash)
> >> >
> >> > I think it would be better if `ParallelApplyTxnHash` is created by
> the first
> >> > successful parallel apply worker.
> >>
> >> Thanks for the suggestion. But I am not sure if it's worth to changing
> the
> >> order here, because It will only optimize the case where user enable
> parallel
> >> apply but never get an available worker which should be rare. And in
> such a
> >> case, it'd be better to increase the number of workers or disable the
> parallel mode.
> >>
> >
> >
> > I think even though the chance is rare, we shouldn't leak resource.
> >
>
> But that is true iff we are never able to start the worker. Anyway, I
> think it is probably fine either way but we can change it as per your
> suggestion to make it more robust and probably for the code clarity
> sake. I'll push this tomorrow unless someone thinks otherwise.
>
> --
> With Regards,
> Amit Kapila.
>

Thanks Amit for the confirmation.

Cheers

Re: GUC for temporarily disabling event triggers

[Ted Yu]

On Thu, Jan 12, 2023 at 12:26 PM Daniel Gustafsson  wrote:

> > On 11 Jan 2023, at 17:38, vignesh C  wrote:
> >
> > On Tue, 29 Nov 2022 at 18:16, Daniel Gustafsson  wrote:
> >>
> >>> On 3 Nov 2022, at 21:47, Daniel Gustafsson  wrote:
> >>
> >>> The patch adds a new GUC, ignore_event_trigger with two option values,
> 'all'
> >>> and 'none' (the login event patch had 'login' as well).
> >>
> >> The attached v2 fixes a small bug which caused testfailures the CFBot.
> >
> > The patch does not apply on top of HEAD as in [1], please post a rebased
> patch:
>
> The attached rebased v3 fixes the conflict.
>
> --
> Daniel Gustafsson   https://vmware.com/
>
> Hi,

`this GUC allows to temporarily suspending event triggers.`

It would be better to mention the name of GUC in the description.
Typo: suspending -> suspend

w.r.t. guc `ignore_event_trigger`, since it is supposed to disable event
triggers for a short period of time, is there mechanism to turn it off
(IGNORE_EVENT_TRIGGER_ALL) automatically ?

Cheers

properly sizing attnums in pg_get_publication_tables

[Ted Yu]

Hi,
I was looking at commit b7ae03953690a1dee455ba3823cc8f71a72cbe1d .

In `pg_get_publication_tables`, attnums is allocated with size
`desc->natts`. However, since some columns may be dropped, this size may be
larger than necessary.
When `nattnums > 0` is false, there is no need to allocate the `attnums`
array. In the current formation, `attnums` should be freed in this scenario.

Please take a look at the patch which moves the allocation to inside the
`if (nattnums > 0)` block.

Thanks


proper-sizing-of-attnums.patch
Description: Binary data

Re: Add tracking of backend memory allocated to pg_stat_activity

[Ted Yu]

On Thu, Aug 31, 2023 at 9:19 AM John Morris 
wrote:

> Here is an updated version of the earlier work.
>
> This version:
>1) Tracks memory as requested by the backend.
>2) Includes allocations made during program startup.
>3) Optimizes the "fast path" to only update two local variables.
>4) Places a cluster wide limit on total memory allocated.
>
> The cluster wide limit is useful for multi-hosting. One greedy cluster
> doesn't starve
> the other clusters of memory.
>
> Note there isn't a good way to track actual memory used by a cluster.
> Ideally, we like to get the working set size of each memory segment along
> with
> the size of the associated kernel data structures.
> Gathering that info in a portable way is a "can of worms".
> Instead, we're managing memory as requested by the application.
> While not identical, the two approaches are strongly correlated.
>
>  The memory model used is
>1) Each process is assumed to use a certain amount of memory
>simply by existing.
>2) All pg memory allocations are counted, including those before
>the process is fully initialized.
> 3) Each process maintains its own local counters. These are the
> "truth".
>  4) Periodically,
> -  local counters are added into the global, shared memory
> counters.
>  - pgstats is updated
>  - total memory is checked.
>
> For efficiency, the global total is an approximation, not a precise number.
> It can be off by as much as 1 MB per process. Memory limiting
> doesn't need precision, just a consistent and reasonable approximation.
>
> Repeating the earlier benchmark test, there is no measurable loss of
> performance.
>
> Hi,
In `InitProcGlobal`:

+elog(WARNING, "proc init: max_total=%d  result=%d\n",
max_total_bkend_mem, result);

Is the above log for debugging purposes ? Maybe the log level should be
changed.

+
errmsg("max_total_backend_memory %dMB - shared_memory_size %dMB is <=
100MB",

The `<=` in the error message is inconsistent with the `max_total_bkend_mem
< result + 100` check.
Please modify one of them.

For update_global_allocation :

+
Assert((int64)pg_atomic_read_u64(&ProcGlobal->total_bkend_mem_bytes) >= 0);
+
Assert((int64)pg_atomic_read_u64(&ProcGlobal->global_dsm_allocation) >= 0);

Should the assertions be done earlier in the function?

For reserve_backend_memory:

+   return true;
+
+   /* CASE: the new allocation is within bounds. Take the fast path. */
+   else if (my_memory.allocated_bytes + size <= allocation_upper_bound)

The `else` can be omitted (the preceding if block returns).

For `atomic_add_within_bounds_i64`

+   newval = oldval + add;
+
+   /* check if we are out of bounds */
+   if (newval < lower_bound || newval > upper_bound ||
addition_overflow(oldval, add))

Since the summation is stored in `newval`, you can pass newval to
`addition_overflow` so that `addition_overflow` doesn't add them again.

There are debug statements, such as:

+   debug("done\n");

you can drop them in the next patch.

Thanks

closing file in adjust_data_dir

[Ted Yu]

Hi,
I was looking at the commit:

commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
Author: Peter Eisentraut 
Date:   Tue Nov 15 15:35:37 2022 +0100

Check return value of pclose() correctly

In src/bin/pg_ctl/pg_ctl.c :

if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL ||
pclose(fd) != 0)

If the fgets() call doesn't return NULL, the pclose() would be skipped.
Since the original pclose() call was removed, wouldn't this lead to fd
leaking ?

Please see attached patch for my proposal.

Cheers


pg-ctl-close-fd.patch
Description: Binary data

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 10:43 AM Ted Yu  wrote:

> Hi,
> I was looking at the commit:
>
> commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
> Author: Peter Eisentraut 
> Date:   Tue Nov 15 15:35:37 2022 +0100
>
> Check return value of pclose() correctly
>
> In src/bin/pg_ctl/pg_ctl.c :
>
> if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL ||
> pclose(fd) != 0)
>
> If the fgets() call doesn't return NULL, the pclose() would be skipped.
> Since the original pclose() call was removed, wouldn't this lead to fd
> leaking ?
>
> Please see attached patch for my proposal.
>
> Cheers
>

There was potential leak of fd in patch v1.

Please take a look at patch v2.

Thanks


pg-ctl-close-fd-v2.patch
Description: Binary data

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 6:02 PM Japin Li  wrote:

>
> On Wed, 16 Nov 2022 at 02:43, Ted Yu  wrote:
> > Hi,
> > I was looking at the commit:
> >
> > commit 2fe3bdbd691a5d11626308e7d660440be6c210c8
> > Author: Peter Eisentraut 
> > Date:   Tue Nov 15 15:35:37 2022 +0100
> >
> > Check return value of pclose() correctly
> >
> > In src/bin/pg_ctl/pg_ctl.c :
> >
> > if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL ||
> > pclose(fd) != 0)
> >
> > If the fgets() call doesn't return NULL, the pclose() would be skipped.
> > Since the original pclose() call was removed, wouldn't this lead to fd
> > leaking ?
> >
> > Please see attached patch for my proposal.
> >
> > Cheers
>
> I think we should check whether fd is NULL or not, otherwise, segmentation
> fault maybe occur.
>
> +   if (pclose(fd) != 0)
> +   {
> +   write_stderr(_("%s: could not close the file following
> command \"%s\"\n"), progname, cmd);
> +   exit(1);
> +   }
>
> Hi,
That check is a few line above:

+   if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
{

Cheers

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 6:35 PM Japin Li  wrote:

>
> On Wed, 16 Nov 2022 at 10:06, Ted Yu  wrote:
> >> Hi,
> > That check is a few line above:
> >
> > +   if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
> > {
> >
> > Cheers
>
> Thanks for the explanation.  Comment on v2 patch.
>
> fd = popen(cmd, "r");
> -   if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL ||
> pclose(fd) != 0)
> +   if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
> {
> +   pclose(fd);
> write_stderr(_("%s: could not determine the data directory
> using command \"%s\"\n"), progname, cmd);
> exit(1);
> }
>
> Here, segfault maybe occurs if fd is NULL.  I think we can remove pclose()
> safely since the process will exit.
>
> --
> Regrads,
> Japin Li.
> ChengDu WenWu Information Technology Co.,Ltd.
>

That means we're going back to v1 of the patch.

Cheers

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 7:12 PM Japin Li  wrote:

>
> On Wed, 16 Nov 2022 at 10:52, Ted Yu  wrote:
> > On Tue, Nov 15, 2022 at 6:35 PM Japin Li  wrote:
> >>
> >> fd = popen(cmd, "r");
> >> -   if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL
> ||
> >> pclose(fd) != 0)
> >> +   if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
> >> {
> >> +   pclose(fd);
> >> write_stderr(_("%s: could not determine the data
> directory
> >> using command \"%s\"\n"), progname, cmd);
> >> exit(1);
> >> }
> >>
> >> Here, segfault maybe occurs if fd is NULL.  I think we can remove
> pclose()
> >> safely since the process will exit.
> >>
> >
> > That means we're going back to v1 of the patch.
> >
>
> After some rethinking, I find the origin code do not have problems.
>
> If fd is NULL or fgets() returns NULL, the process exits.  Otherwise, we
> call
> pclose() to close fd.  The code isn't straightforward, however, it is
> correct.
>
>
>
> Please read this sentence from my first post:

If the fgets() call doesn't return NULL, the pclose() would be skipped.

Re: closing file in adjust_data_dir

[Ted Yu]

On Tue, Nov 15, 2022 at 7:26 PM Japin Li  wrote:

>
> On Wed, 16 Nov 2022 at 11:15, Ted Yu  wrote:
> > On Tue, Nov 15, 2022 at 7:12 PM Japin Li  wrote:
> >> After some rethinking, I find the origin code do not have problems.
> >>
> >> If fd is NULL or fgets() returns NULL, the process exits.  Otherwise, we
> >> call
> >> pclose() to close fd.  The code isn't straightforward, however, it is
> >> correct.
>
> Hi,
Please take a look at the following:

https://en.cppreference.com/w/c/io/fgets

Quote: If the failure has been caused by some other error, sets the
*error* indicator
(see ferror() <https://en.cppreference.com/w/c/io/ferror>) on stream. The
contents of the array pointed to by str are indeterminate (it may not even
be null-terminated).

I think we shouldn't assume that the fd doesn't need to be closed when NULL
is returned from fgets().

Cheers

Re: closing file in adjust_data_dir

[Ted Yu]

On Wed, Nov 16, 2022 at 12:28 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 16.11.22 04:31, Ted Yu wrote:
> > On Wed, 16 Nov 2022 at 11:15, Ted Yu  > <mailto:yuzhih...@gmail.com>> wrote:
> >  > On Tue, Nov 15, 2022 at 7:12 PM Japin Li  > <mailto:japi...@hotmail.com>> wrote:
> >  >> After some rethinking, I find the origin code do not have
> problems.
> >  >>
> >  >> If fd is NULL or fgets() returns NULL, the process exits.
> > Otherwise, we
> >  >> call
> >  >> pclose() to close fd.  The code isn't straightforward, however,
> > it is
> >  >> correct.
> >
> > Hi,
> > Please take a look at the following:
> >
> > https://en.cppreference.com/w/c/io/fgets
> > <https://en.cppreference.com/w/c/io/fgets>
> > Quote: If the failure has been caused by some other error, sets the
> > /error/ indicator (see ferror()
> > <https://en.cppreference.com/w/c/io/ferror>) on |stream|. The contents
> > of the array pointed to by |str| are indeterminate (it may not even be
> > null-terminated).
>
> That has nothing to do with the return value of fgets().
>
> Hi, Peter:
Here is how the return value from pclose() is handled in other places:

+   if (pclose_rc != 0)
+   {
+   ereport(ERROR,

The above is very easy to understand.
While the check in `adjust_data_dir` is somewhat harder to comprehend.

I think the formation presented in patch v1 aligns with existing checks of
the return value from pclose().
It also gives a unique error message in the case that the return value from
pclose() indicates an error.

Cheers

redundant check of msg in does_not_exist_skipping

[Ted Yu]

Hi,
I was looking at commit aca992040951c7665f1701cd25d48808eda7a809

I think the check of msg after the switch statement is not necessary. The
variable msg is used afterward.
If there is (potential) missing case in switch statement, the compiler
would warn.

How about removing the check ?

Thanks

diff --git a/src/backend/commands/dropcmds.c
b/src/backend/commands/dropcmds.c
index db906f530e..55996940eb 100644
--- a/src/backend/commands/dropcmds.c
+++ b/src/backend/commands/dropcmds.c
@@ -518,9 +518,6 @@ does_not_exist_skipping(ObjectType objtype, Node
*object)

 /* no default, to let compiler warn about missing case */
 }
-if (!msg)
-elog(ERROR, "unrecognized object type: %d", (int) objtype);
-
 if (!args)
 ereport(NOTICE, (errmsg(msg, name)));
 else

Re: Getting rid of SQLValueFunction

[Ted Yu]

On Sat, Nov 19, 2022 at 7:01 PM Michael Paquier  wrote:

> On Fri, Nov 18, 2022 at 10:23:58AM +0900, Michael Paquier wrote:
> > Please note that in order to avoid tweaks when choosing the attribute
> > name of function call, this needs a total of 8 new catalog functions
> > mapping to the SQL keywords, which is what the test added by 2e0d80c
> > is about:
> > - current_role
> > - user
> > - current_catalog
> > - current_date
> > - current_time
> > - current_timestamp
> > - localtime
> > - localtimestamp
> >
> > Any objections?
>
> Hearing nothing, I have gone through 0001 again and applied it as
> fb32748 to remove the dependency between names and SQLValueFunction.
> Attached is 0002, to bring back the CI to a green state.
> --
> Michael
>

Hi,
For get_func_sql_syntax(), the code for cases
of F_CURRENT_TIME, F_CURRENT_TIMESTAMP, F_LOCALTIME and F_LOCALTIMESTAMP is
mostly the same.
Maybe we can introduce a helper so that code duplication is reduced.

Cheers

Re: Getting rid of SQLValueFunction

[Ted Yu]

On Sun, Nov 20, 2022 at 3:12 PM Michael Paquier  wrote:

> On Sun, Nov 20, 2022 at 08:21:10AM -0800, Ted Yu wrote:
> > For get_func_sql_syntax(), the code for cases
> > of F_CURRENT_TIME, F_CURRENT_TIMESTAMP, F_LOCALTIME and F_LOCALTIMESTAMP
> is
> > mostly the same.
> > Maybe we can introduce a helper so that code duplication is reduced.
>
> It would.  Thanks for the suggestion.
>
> Do you like something like the patch 0002 attached?  This reduces a
> bit the overall size of the patch.  Both ought to be merged in the
> same commit, still it is easier to see the simplification created.
> --
> Michael
>
Hi,
Thanks for the quick response.

+ * timestamp.  These require a specific handling with their typmod is given
+ * by the function caller through their SQL keyword.

typo: typmod is given -> typmod given

Other than the above, code looks good to me.

Cheers

Re: Partial aggregates pushdown

[Ted Yu]

On Mon, Nov 21, 2022 at 5:02 PM fujii.y...@df.mitsubishielectric.co.jp <
fujii.y...@df.mitsubishielectric.co.jp> wrote:

> Hi Mr.Vondra, Mr.Pyhalov, Everyone.
>
> I discussed with Mr.Pyhalov about the above draft by directly sending mail
> to
>  him(outside of pgsql-hackers). Mr.Pyhalov allowed me to update his patch
> along with the above draft. So I update Mr.Pyhalov's patch v10.
>
> I wrote my patch for discussion.
> My patch passes regression tests which contains additional basic
> postgres_fdw tests
> for my patch's feature. But my patch doesn't contain sufficient documents
> and tests.
> If reviewers accept my approach, I will add documents and tests to my
> patch.
>
> The following is a my patch's readme.
> # I simplified the above draft.
>
> --readme of my patch
> 1. interface
> 1) pg_aggregate
> There are the following additional columns.
> a) partialaggfn
>   data type: regproc.
>   default value: zero(means invalid).
>   description  : This field refers to the special aggregate function(then
> we call
>  this partialaggfunc)
> corresponding to aggregation function(then we call src) which has
> aggfnoid.
> partialaggfunc is used for partial aggregation pushdown by
> postgres_fdw.
> The followings are differences between the src and the special
> aggregate function.
>   difference1) result type
> The result type is same as the src's transtype if the src's
> transtype
> is not internal.
> Otherwise the result type is bytea.
>   difference2) final func
> The final func does not exist if the src's transtype is not
> internal.
> Otherwize the final func returns serialized value.
> For example, there is a partialaggfunc avg_p_int4 which corresponds to
> avg(int4)
> whose aggtranstype is _int4.
> The result value of avg_p_int4 is a float8 array which consists of
> count and
> summation. avg_p_int4 does not have finalfunc.
> For another example, there is a partialaggfunc avg_p_int8 which
> corresponds to
> avg(int8) whose aggtranstype is internal.
> The result value of avg_p_int8 is a bytea serialized array which
> consists of count
> and summation. avg_p_int8 has finalfunc int8_avg_serialize which is
> serialize function
> of avg(int8). This field is zero if there is no partialaggfunc.
>
> b) partialagg_minversion
>   data type: int4.
>   default value: zero(means current version).
>   description  : This field is the minimum PostgreSQL server version which
> has
> partialaggfunc. This field is used for checking compatibility of
> partialaggfunc.
>
> The above fields are valid in tuples for builtin avg, sum, min, max, count.
> There are additional records which correspond to partialaggfunc for avg,
> sum, min, max,
> count.
>
> 2) pg_proc
> There are additional records which correspond to partialaggfunc for avg,
> sum, min, max,
> count.
>
> 3) postgres_fdw
> postgres_fdw has an additional foreign server option server_version.
> server_version is
> integer value which means remote server version number. Default value of
> server_version
> is zero. server_version is used for checking compatibility of
> partialaggfunc.
>
> 2. feature
> postgres_fdw can pushdown partial aggregation of avg, sum, min, max, count.
> Partial aggregation pushdown is fine when the following two conditions are
> both true.
>   condition1) partialaggfn is valid.
>   condition2) server_version is not less than partialagg_minversion
> postgres_fdw executes pushdown the patialaggfunc instead of a src.
> For example, we issue "select avg_p_int4(c) from t" instead of "select
> avg(c) from t"
> in the above example.
>
> postgres_fdw can pushdown every aggregate function which supports partial
> aggregation
> if you add a partialaggfunc corresponding to the aggregate function by
> create aggregate
> command.
>
> 3. difference between my patch and Mr.Pyhalov's v10 patch.
> 1) In my patch postgres_fdw can pushdown partial aggregation of avg
> 2) In my patch postgres_fdw can pushdown every aggregate function which
> supports partial
>   aggregation if you add a partialaggfunc corresponding to the aggregate
> function.
>
> 4. sample commands in psql
> \c postgres
> drop database tmp;
> create database tmp;
> \c tmp
> create extension postgres_fdw;
> create server server_01 foreign data wrapper postgres_fdw options(host
> 'localhost', dbname 'tmp', server_version '16', async_capable 'true');
> create user mapping for postgres server server_01 options(user 'postgres',
> password 'postgres');
> create server server_02 foreign data wrapper postgres_fdw options(host
> 'localhost', dbname 'tmp', server_version '16', async_capable 'true');
> create user mapping for postgres server server_02 options(user 'postgres',
> password 'postgres');
>
> create table t(dt timestamp, id int4, name text, total int4, val float4,
> type int4, span interval) partition by list (type);
>
> create table t1(dt timestamp, id int4, name text, total int4

Re: [CAUTION!! freemail] Re: Partial aggregates pushdown

[Ted Yu]

On Tue, Nov 22, 2022 at 1:11 AM fujii.y...@df.mitsubishielectric.co.jp <
fujii.y...@df.mitsubishielectric.co.jp> wrote:

> Hi Mr.Yu.
>
> Thank you for comments.
>
> > + * Check that partial aggregate agg has compatibility
> >
> > If the `agg` refers to func parameter, the parameter name is aggform
> I fixed the above typo and made the above comment easy to understand
> New comment is "Check that partial aggregate function of aggform exsits in
> remote"
>
> > +   int32  partialagg_minversion = PG_VERSION_NUM;
> > +   if (aggform->partialagg_minversion ==
> > PARTIALAGG_MINVERSION_DEFAULT) {
> > +   partialagg_minversion = PG_VERSION_NUM;
> >
> >
> > I am curious why the same variable is assigned the same value twice. It
> seems
> > the if block is redundant.
> >
> > +   if ((fpinfo->server_version >= partialagg_minversion)) {
> > +   compatible = true;
> >
> >
> > The above can be simplified as: return fpinfo->server_version >=
> > partialagg_minversion;
> I fixed according to your comment.
>
> Sincerely yours,
> Yuuki Fujii
>
>
> Hi,
Thanks for the quick response.

cleanup in open_auth_file

[Ted Yu]

Hi,
I was looking at the following commit:

commit efc981627a723d91e86865fb363d793282e473d1
Author: Michael Paquier 
Date:   Thu Nov 24 08:21:55 2022 +0900

Rework memory contexts in charge of HBA/ident tokenization

I think when the file cannot be opened, the context should be deleted.

Please see attached patch.

I also modified one comment where `deleted` would be more appropriate verb
for the context.

Cheers


open-auth-file-cleanup.patch
Description: Binary data

Re: cleanup in open_auth_file

[Ted Yu]

On Wed, Nov 23, 2022 at 4:54 PM Ted Yu  wrote:

> Hi,
> I was looking at the following commit:
>
> commit efc981627a723d91e86865fb363d793282e473d1
> Author: Michael Paquier 
> Date:   Thu Nov 24 08:21:55 2022 +0900
>
> Rework memory contexts in charge of HBA/ident tokenization
>
> I think when the file cannot be opened, the context should be deleted.
>
> Please see attached patch.
>
> I also modified one comment where `deleted` would be more appropriate verb
> for the context.
>
> Cheers
>

Thinking more on this.
The context should be created when the file is successfully opened.

Please take a look at patch v2.


open-auth-file-cleanup-v2.patch
Description: Binary data

indentation in _hash_pgaddtup()

[Ted Yu]

Hi,
I was looking at :

commit d09dbeb9bde6b9faabd30e887eff4493331d6424
Author: David Rowley 
Date:   Thu Nov 24 17:21:44 2022 +1300

Speedup hash index builds by skipping needless binary searches

In _hash_pgaddtup(), it seems the indentation is off for the assertion.

Please take a look at the patch.

Thanks


hash-pgaddtup-indent.patch
Description: Binary data

Re: indentation in _hash_pgaddtup()

[Ted Yu]

On Thu, Nov 24, 2022 at 7:11 AM Tom Lane  wrote:

> Daniel Gustafsson  writes:
> >> On 24 Nov 2022, at 13:42, Ted Yu  wrote:
> >> In _hash_pgaddtup(), it seems the indentation is off for the assertion.
>
> > Indentation is handled by applying src/tools/pgindent to the code, and
> > re-running it on this file yields no re-indentation so this is in fact
> correct
> > according to the pgindent rules.
>
> It is one messy bit of code though --- perhaps a little more thought
> about where to put line breaks would help?  Alternatively, it could
> be split into multiple statements, along the lines of
>
> #ifdef USE_ASSERT_CHECKING
> if (PageGetMaxOffsetNumber(page) > 0)
> {
> IndexTuple lasttup = PageGetItem(page,
>  PageGetItemId(page,
>
>  PageGetMaxOffsetNumber(page)));
>
> Assert(_hash_get_indextuple_hashkey(lasttup) <=
>_hash_get_indextuple_hashkey(itup));
> }
> #endif
>
> (details obviously tweakable)
>
> regards, tom lane
>

Thanks Tom for the suggestion.

Here is patch v2.


hash-pgaddtup-indent-v2.patch
Description: Binary data

Re: indentation in _hash_pgaddtup()

[Ted Yu]

On Thu, Nov 24, 2022 at 12:31 PM Tom Lane  wrote:

> David Rowley  writes:
> > After running pgindent on v2, I see it still pushes the lines out
> > quite far. If I add a new line after PageGetItemId(page, and put the
> > variable assignment away from the variable declaration then it looks a
> > bit better. It's still 1 char over the limit.
>
> If you wanted to be hard-nosed about 80 character width, you could
> pull out the PageGetItemId call into a separate local variable.
> I wasn't going to be quite that picky, but I won't object if that
> seems better to you.
>
> regards, tom lane
>

Patch v4 stores ItemId in a local variable.


hash-pgaddtup-indent-v4.patch
Description: Binary data

checking rd_rules in RelationBuildDesc

[Ted Yu]

Hi,
(In light of commit 7b2ccc5e03bf16d1e1bbabca25298108c839ec52)

In RelationBuildDesc(), we have:

if (relation->rd_rel->relhasrules)
RelationBuildRuleLock(relation);

I wonder if we should check relation->rd_rules after the call
to RelationBuildRuleLock().

Your comment is appreciated.


build-desc-check-rules.patch
Description: Binary data

Re: checking rd_rules in RelationBuildDesc

[Ted Yu]

On Fri, Nov 25, 2022 at 8:17 AM Tom Lane  wrote:

> Ted Yu  writes:
> > I wonder if we should check relation->rd_rules after the call
> > to RelationBuildRuleLock().
>
> That patch is both pointless and wrong.  There is some
> value in updating relhasrules in the catalog, so that future
> relcache loads don't uselessly call RelationBuildRuleLock;
> but we certainly can't try to do so right there.  That being
> the case, making the relcache be out of sync with what's on
> disk cannot have any good consequences.  The most likely
> effect is that it would block later logic from fixing things
> correctly.  There is logic in VACUUM to clean out obsolete
> relhasrules flags (see vac_update_relstats), but I suspect
> that would no longer work properly if we did this.
>
> regards, tom lane
>
Hi,
Thanks for evaluating the patch.

The change was originating from what we have in
RelationCacheInitializePhase3():

if (relation->rd_rel->relhasrules && relation->rd_rules ==
NULL)
{
RelationBuildRuleLock(relation);
if (relation->rd_rules == NULL)
relation->rd_rel->relhasrules = false;

FYI

Re: Add tracking of backend memory allocated to pg_stat_activity

[Ted Yu]

On Sat, Nov 26, 2022 at 9:32 PM Reid Thompson 
wrote:

> On Sat, 2022-11-26 at 22:10 -0500, Reid Thompson wrote:
> > Code rebased to current master.
> > Updated to incorporate additional recommendations from the the list
> >- add units to variables in view
> >- remove 'backend_' prefix from variables/functions
> >- update documentation
> >- add functional test for allocated_bytes
> >- refactor allocation reporting to reduce number of functions and
> >  branches/reduce performance hit
> >- zero allocated bytes after fork to avoid double counting
> > postmaster allocations
> >
> >
> >
> >
>
> attempt to remedy cfbot windows build issues
>
>
> Hi,

+   if (request_size > *mapped_size)
+   {
+   pgstat_report_allocated_bytes(*mapped_size,
DECREASE);
+   pgstat_report_allocated_bytes(request_size,
INCREASE);

pgstat_report_allocated_bytes is called twice for this case. Can the two
calls be combined into one (with request_size - *mapped_size, INCREASE) ?

Cheers

Re: Add tracking of backend memory allocated to pg_stat_activity

[Ted Yu]

On Sun, Nov 27, 2022 at 7:41 AM Justin Pryzby  wrote:

> On Sun, Nov 27, 2022 at 12:32:19AM -0500, Reid Thompson wrote:
> > @@ -32,6 +33,12 @@ typedef enum BackendState
> >   STATE_DISABLED
> >  } BackendState;
> >
> > +/* Enum helper for reporting memory allocated bytes */
> > +enum allocation_direction
> > +{
> > + DECREASE = -1,
> > + INCREASE = 1,
> > +};
>
> BTW, these should have some kind of prefix, like PG_ALLOC_* to avoid
> causing the same kind of problem for someone else that another header
> caused for you by defining something somewhere called IGNORE (ignore
> what, I don't know).  The other problem was probably due to a define,
> though.  Maybe instead of an enum, the function should take a boolean.
>
> I still wonder whether there needs to be a separate CF entry for the
> 0001 patch.  One issue is that there's two different lists of people
> involved in the threads.
>
> --
> Justin
>
>
> I am a bit curious: why is the allocation_direction enum needed ?

pgstat_report_allocated_bytes() can be given the amount (either negative or
positive) to adjust directly.

Cheers

Re: Operation log for major operations

[Ted Yu]

On Thu, Jan 19, 2023 at 1:12 PM Dmitry Koval  wrote:

>  >The patch does not apply on top of HEAD ...
>
> Thanks!
> Here is a fixed version.
>
> Additional changes:
> 1) get_operation_log() function doesn't create empty operation log file;
> 2) removed extra unlink() call.
>
> --
> With best regards,
> Dmitry Koval
>
> Postgres Professional: http://postgrespro.com

Hi,

Copyright (c) 1996-2022

Please update year for the license in pg_controllog.c

+check_file_exists(const char *datadir, const char *path)

There is existing helper function such as:

src/backend/utils/fmgr/dfmgr.c:static bool file_exists(const char *name);

Is it possible to reuse that code ?

Cheers

Re: Operation log for major operations

[Ted Yu]

On Fri, Jan 20, 2023 at 1:19 AM Dmitry Koval  wrote:

> Thanks, Ted Yu!
>
>  > Please update year for the license in pg_controllog.c
>
> License updated for files pg_controllog.c, controllog_utils.c,
> pg_controllog.h, controllog_utils.h.
>
>  > +check_file_exists(const char *datadir, const char *path)
>  > There is existing helper function such as:
>  > src/backend/utils/fmgr/dfmgr.c:static bool file_exists(const char
> *name);
>  > Is it possible to reuse that code ?
>
> There are a lot of functions that check the file existence:
>
> 1) src/backend/utils/fmgr/dfmgr.c:static bool file_exists(const char
> *name);
> 2) src/backend/jit/jit.c:static bool file_exists(const char *name);
> 3) src/test/regress/pg_regress.c:bool file_exists(const char *file);
> 4) src/bin/pg_upgrade/exec.c:bool pid_lock_file_exists(const char
> *datadir);
> 5) src/backend/commands/extension.c:bool extension_file_exists(const
> char *extensionName);
>
> But there is no unified function: different components use their own
> function with their own specific.
> Probably we can not reuse code of dfmgr.c:file_exists() because this
> function skip "errno == EACCES" (this error is critical for us).
> I copied for src/bin/pg_rewind/file_ops.c:check_file_exists() code of
> function jit.c:file_exists() (with adaptation for the utility).
>
> --
> With best regards,
> Dmitry Koval
>
> Postgres Professional: http://postgrespro.com

Hi,
Maybe another discussion thread can be created for the consolidation of
XX_file_exists functions.

Cheers

Re: Named Operators

[Ted Yu]

On Fri, Jan 20, 2023 at 9:17 AM Gurjeet Singh  wrote:

> On Sat, Jan 14, 2023 at 6:14 AM Gurjeet Singh  wrote:
> >
> > I agree that an identifier _surrounded_ by the same token (e.g. #foo#)
> > or the pairing token (e.g. {foo}) looks better aesthetically, so I am
> > okay with any of the following variations of the scheme, as well:
> >
> > \#foo\#  (tested; works)
> > \#foo#   (not tested; reduces ident length by 1)
> >
> > We can choose a different character, instead of #. Perhaps \{foo} !
>
> Please find attached the patch that uses \{foo} styled Named
> Operators. This is in line with Tom's reluctant hint at possibly using
> curly braces as delimiter characters. Since the curly braces are used
> by the SQL Specification for row pattern recognition, this patch
> proposes escaping the first of the curly braces.
>
> We can get rid of the leading backslash, if (a) we're confident that
> SQL committee will not use curly braces anywhere else, and (b) if
> we're confident that if/when Postgres supports Row Pattern Recognition
> feature, we'll be able to treat curly braces inside the PATTERN clause
> specially. Since both of those conditions are unlikely, I think we
> must settle for the escaped-first-curly-brace style for the naming our
> operators.
>
> Keeping with the previous posts, here's a sample SQL script showing
> what the proposed syntax will look like in action. Personally, I
> prefer the \#foo style, since the \# prefix stands out among the text,
> better than \{..} does, and because # character is a better signal of
> an operator than {.
>
> create operator \{add_point}
> (function = box_add, leftarg = box, rightarg = point);
> create table test(a box);
> insert into test values('((0,0),(1,1))'), ('((0,0),(2,1))');
> select a as original, a \{add_point} '(1,1)' as modified from test;
> drop operator \{add_point}(box, point);
>
> Best regards,
> Gurjeet
> http://Gurje.et


Hi,
Since `validIdentifier` doesn't modify the contents of `name` string, it
seems that there is no need to create `tmp` string in `validNamedOperator`.
You can pass the start and end offsets into the string (name) as second and
third parameters to `validIdentifier`.

Cheers

Re: bug: copy progress reporting of backends which run multiple COPYs

[Ted Yu]

On Fri, Jan 20, 2023 at 4:51 PM Matthias van de Meent <
boekewurm+postg...@gmail.com> wrote:

> On Thu, 19 Jan 2023 at 06:47, Justin Pryzby  wrote:
> >
> > pg_stat_progress_copy was added in v14 (8a4f618e7, 9d2d45700).
> >
> > But if a command JOINs file_fdw tables, the progress report gets bungled
> > up.  This will warn/assert during file_fdw tests.
>
> I don't know what to do with that other than disabling COPY progress
> reporting for file_fdw, i.e. calls to BeginCopyFrom that don't supply
> a pstate. This is probably the best option, because a table backed by
> file_fdw would also interfere with COPY TO's progress reporting.
>
> Attached a patch that solves this specific issue in a
> binary-compatible way. I'm not super happy about relying on behavior
> of callers of BeginCopyFrom (assuming that users that run copy
> concurrently will not provide a ParseState* to BeginCopyFrom), but it
> is what it is.
>
> Kind regards,
>
> Matthias van de Meent
>
Hi,
In `BeginCopyFrom`, I see the following :

if (pstate)
{
cstate->range_table = pstate->p_rtable;
cstate->rteperminfos = pstate->p_rteperminfos;

Is it possible to check range_table / rteperminfos so that we don't
introduce the bool field ?

Cheers

Re: [Proposal] Add foreign-server health checks infrastructure

[Ted Yu]

On Sat, Jan 21, 2023 at 4:03 AM Hayato Kuroda (Fujitsu) <
kuroda.hay...@fujitsu.com> wrote:

> Dear Katsuragi-san,
>
> Thank you for reviewing! PSA new patch set.
>
> > ## v24-0001-Add-PQConncheck-and-PQCanConncheck-to-libpq.patch
> > +
> > +
> > PQCanConncheckPQCan
> > Conncheck
> > + 
> > +  
> > +   Returns the status of the socket.
> >
> > Is this description right? I think this description is for
> > PQConncheck. Something like "Checks whether PQConncheck is
> > available on this platform." seems better.
>
> It might be copy-and-paste error. Thanks for reporting.
> According to other parts, the sentence should be started like "Returns
> ...".
> So I followed the style and did cosmetic change.
>
> > +/* Check whether the postgres server is still alive or not */
> > +extern int PQConncheck(PGconn *conn);
> > +extern int PQCanConncheck(void);
> >
> > Should the names of these functions be in the form of PQconnCheck?
> > Not PQConncheck (c.f. The section of fe-misc.c in libpq-fe.h).
>
> Agreed, fixed.
>
> > ## v24-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch
> > +PG_FUNCTION_INFO_V1(postgres_fdw_verify_connection_states);
> > +PG_FUNCTION_INFO_V1(postgres_fdw_verify_connection_states_all);
> > +PG_FUNCTION_INFO_V1(postgres_fdw_can_verify_connection_states);
> >
> > This patch adds new functions to postgres_fdw for PostgreSQL 16.
> > So, I think it is necessary to update the version of postgres_fdw (v1.1
> > to v1.2).
>
> I checked postgres_fdw commit log, and it seemed that the version was
> updated when SQL functions are added. Fixed.
>
> > +postgres_fdw_verify_connection_states_all() returns
> > boolean
> > +
> > + 
> > +  This function checks the status of remote connections that are
> > established
> > +  by postgres_fdw from the local session to
> > the foreign
> > +  servers. This check is performed by polling the socket and allows
> >
> > It seems better to add a description that states this function
> > checks all the connections. Like the description of
> > postgres_fdw_disconnect_all(). For example, "This function
> > checks the status of 'all the' remote connections..."?
>
> I checked the docs and fixed. Moreover, some inconsistent statements were
> also fixed.
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
>
> Hi,
For v25-0001-Add-PQConnCheck-and-PQCanConnCheck-to-libpq.patch ,
`pqConnCheck_internal` only has one caller which is quite short.
Can pqConnCheck_internal and PQConnCheck be merged into one func ?

+int
+PQCanConnCheck(void)

It seems the return value should be of bool type.

Cheers

Re: MERGE ... WHEN NOT MATCHED BY SOURCE

[Ted Yu]

On Sat, Jan 21, 2023 at 3:05 AM Dean Rasheed 
wrote:

> On Tue, 10 Jan 2023 at 14:43, Dean Rasheed 
> wrote:
> >
> > Rebased version attached.
> >
>
> Rebased version, following 8eba3e3f02 and 5d29d525ff.
>
> Regards,
> Dean
>
Hi,
In transform_MERGE_to_join :

+   if (action->matchKind ==
MERGE_WHEN_NOT_MATCHED_BY_SOURCE)
+   tgt_only_tuples = true;
+   if (action->matchKind ==
MERGE_WHEN_NOT_MATCHED_BY_TARGET)

There should be an `else` in front of the second `if`.
When tgt_only_tuples and src_only_tuples are both true, we can come out of
the loop.

Cheers