Re: Need clarification on compilation errors in PG 16.2

[Pradeep Kumar]

Hi Long,

>In fact, whether the HAVE_MEMSET_S macro is defined determines whether the
implementation
>of the explicit_bzero() function calls memset_s() or memset(). This macro
is undefined by default
>in pg_config.h, so check to see if your build environment has a
HAVE_MEMSET_S macro defined.

Yes it was defined in "pg_config.h"
/* Define to 1 if you have the `memset_s' function. */
#define HAVE_MEMSET_S 1

Thanks

On Tue, May 28, 2024 at 12:27 PM Long Song  wrote:

>
>
>
>
>
> Hi Pradeep,
>
>
> At 2024-05-28 12:37:08, "Pradeep Kumar"  wrote:
>
> > Hi ,
> > While we try to install PG 16.2 from the source code in macbook we
> getting this following errors
> > ```
> >
> > explicit_bzero.c:22:9: error: call to undeclared function 'memset_s';
> ISO C99 and later do not support implicit function declarations
> [-Wimplicit-function-declaration]
> > (void) memset_s(buf, len, 0, len);
> >^
> > explicit_bzero.c:22:9: note: did you mean 'memset'?
> >
> /Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/string.h:74:7:
> note: 'memset' declared here
> > voidmemset(void __b, int __c, size_t __len);
> >  ^
> > 1 error generated.
> > make[2]: *** [explicit_bzero.o] Error 1
> > make[2]: *** Waiting for unfinished jobs
> > make[1]: *** [all-port-recurse] Error 2
> > make: *** [all-src-recurse] Error 2
> >
> > ```
> > then I changed the function memset_s(buf, len, 0, len) to memset(buf, 0,
> len) and it's working. need a clarification on this?
> In fact, whether the HAVE_MEMSET_S macro is defined determines whether the
> implementation
> of the explicit_bzero() function calls memset_s() or memset(). This macro
> is undefined by default
> in pg_config.h, so check to see if your build environment has a
> HAVE_MEMSET_S macro defined.
>
> Best Regards,
> Long

why memoize is not used for correlated subquery

[Pavel Stehule]

Hi

I am playing with examples for P2D2, and I found few issues related to
memoize

1. I use dataset https://pgsql.cz/files/obce.sql - it is data about czech
population

Dictionary - "obec" -> "village", "pocet_muzu" -> "number_of_men",
"pocet_zen" -> "number_of_woman", "okres" -> "district", "nazev" -> "name"

I wrote the query - biggest village per district

select nazev
  from obce o
  where pocet_muzu + pocet_zen = (select max(pocet_muzu + pocet_zen)
from obce
   where o.okres_id = okres_id);



I expected usage of memoize, because in this query, it can be very
effective https://explain.depesz.com/s/0ubC

(2024-05-28 09:09:58) postgres=# explain select nazev from obce o where
pocet_muzu + pocet_zen = (select max(pocet_muzu + pocet_zen) from obce
where o.okres_id = okres_id);
QUERY PLAN

══
Seq Scan on obce o  (cost=0.00..33588.33 rows=31 width=10)
  Filter: ((pocet_muzu + pocet_zen) = (SubPlan 2))
  SubPlan 2
->  Result  (cost=5.34..5.35 rows=1 width=4)
  InitPlan 1
->  Limit  (cost=0.28..5.34 rows=1 width=4)
  ->  Index Scan Backward using obce_expr_idx on obce
 (cost=0.28..409.92 rows=81 width=4)
Index Cond: ((pocet_muzu + pocet_zen) IS NOT NULL)
Filter: ((o.okres_id)::text = (okres_id)::text)
(9 rows)

But it doesn't do. I rewrote this query to lateral join, and memoize was
used, but the result was not good, because filter wa pushed to subquery

explain select * from obce o, lateral (select max(pocet_zen + pocet_muzu)
from obce where o.okres_id = okres_id) where pocet_zen + pocet_muzu = max;
  QUERY PLAN

══
Nested Loop  (cost=12.83..19089.82 rows=31 width=45)
  ->  Seq Scan on obce o  (cost=0.00..121.50 rows=6250 width=41)
  ->  Memoize  (cost=12.83..12.85 rows=1 width=4)
Cache Key: (o.pocet_zen + o.pocet_muzu), o.okres_id
Cache Mode: binary
->  Subquery Scan on unnamed_subquery  (cost=12.82..12.84 rows=1
width=4)
  Filter: ((o.pocet_zen + o.pocet_muzu) = unnamed_subquery.max)
  ->  Aggregate  (cost=12.82..12.83 rows=1 width=4)
->  Index Scan using obce_okres_id_idx on obce
 (cost=0.28..12.41 rows=81 width=8)
  Index Cond: ((okres_id)::text =
(o.okres_id)::text)
(10 rows)

and then the effect of memoize is almost negative
https://explain.depesz.com/s/TKLL

When I used optimization fence, then memoize was used effectively
https://explain.depesz.com/s/hhgi

explain select * from (select * from obce o, lateral (select max(pocet_zen
+ pocet_muzu) from obce where o.okres_id = okres_id) offset 0) where
pocet_zen + pocet_muzu = max;
  QUERY PLAN

══
Subquery Scan on unnamed_subquery  (cost=12.83..1371.93 rows=31 width=45)
  Filter: ((unnamed_subquery.pocet_zen + unnamed_subquery.pocet_muzu) =
unnamed_subquery.max)
  ->  Nested Loop  (cost=12.83..1278.18 rows=6250 width=45)
->  Seq Scan on obce o  (cost=0.00..121.50 rows=6250 width=41)
->  Memoize  (cost=12.83..12.84 rows=1 width=4)
  Cache Key: o.okres_id
  Cache Mode: binary
  ->  Aggregate  (cost=12.82..12.83 rows=1 width=4)
->  Index Scan using obce_okres_id_idx on obce
 (cost=0.28..12.41 rows=81 width=8)
  Index Cond: ((okres_id)::text =
(o.okres_id)::text)
(10 rows)

My question is - does memoize support subqueries? And can be enhanced to
support this exercise without LATERAL and optimization fences?

Regards

Pavel

Re: why memoize is not used for correlated subquery

[Tender Wang]

Pavel Stehule  于2024年5月28日周二 15:31写道：

> Hi
>
>
> My question is - does memoize support subqueries? And can be enhanced to
> support this exercise without LATERAL and optimization fences?
>
>
The commit messages in memoize may answer your question:

   "For now, the planner will only consider using a result cache for
parameterized nested loop joins.  This works for both normal joins and
also for LATERAL type joins to subqueries.  It is possible to use this
new
node for other uses in the future.  For example, to cache results from
correlated subqueries.  However, that's not done here due to some
difficulties obtaining a distinct estimation on the outer plan to
calculate the estimated cache hit ratio.  Currently we plan the inner
plan
before planning the outer plan so there is no good way to know if a
result
cache would be useful or not since we can't estimate the number of times
the subplan will be called until the outer plan is generated."

git show b6002a796d
-- 
Tender Wang
OpenPie:  https://en.openpie.com/

Re: why memoize is not used for correlated subquery

[David Rowley]

On Tue, 28 May 2024 at 19:31, Pavel Stehule  wrote:
> My question is - does memoize support subqueries? And can be enhanced to 
> support this exercise without LATERAL and optimization fences?

It's only currently considered for parameterized nested loop joins,
not for subplans.

I wrote a bit about this in [1] and there's even a patch.  The problem
with it is that we plan subqueries and generate an actual plan before
planning the outer query.  This means we don't have an ndistinct
estimate for the parameters to the subquery when we plan it, therefore
we can't tell if Memoize is a good choice or not.  It isn't a good
choice if each set of parameters the subplan is called with is unique.
That would always be a cache miss and would only result in making the
query run more slowly.

I imagined making this work by delaying the plan creation for
subqueries until the same time as create_plan() for the outer query.
If we have a Path with and without a Memoize node, at some point after
planning the outer query, we can choose which Path is the cheapest
based on the ndistinct estimate for the parameters.

David

[1] 
https://www.postgresql.org/message-id/CAApHDvpGX7RN%2Bsh7Hn9HWZQKp53SjKaL%3DGtDzYheHWiEd-8moQ%40mail.gmail.com

Re: why memoize is not used for correlated subquery

[Pavel Stehule]

út 28. 5. 2024 v 9:48 odesílatel David Rowley  napsal:

> On Tue, 28 May 2024 at 19:31, Pavel Stehule 
> wrote:
> > My question is - does memoize support subqueries? And can be enhanced to
> support this exercise without LATERAL and optimization fences?
>
> It's only currently considered for parameterized nested loop joins,
> not for subplans.
>
> I wrote a bit about this in [1] and there's even a patch.  The problem
> with it is that we plan subqueries and generate an actual plan before
> planning the outer query.  This means we don't have an ndistinct
> estimate for the parameters to the subquery when we plan it, therefore
> we can't tell if Memoize is a good choice or not.  It isn't a good
> choice if each set of parameters the subplan is called with is unique.
> That would always be a cache miss and would only result in making the
> query run more slowly.
>
> I imagined making this work by delaying the plan creation for
> subqueries until the same time as create_plan() for the outer query.
> If we have a Path with and without a Memoize node, at some point after
> planning the outer query, we can choose which Path is the cheapest
> based on the ndistinct estimate for the parameters.
>

Thank you for explanation

Pavel

>
> David
>
> [1]
> https://www.postgresql.org/message-id/CAApHDvpGX7RN%2Bsh7Hn9HWZQKp53SjKaL%3DGtDzYheHWiEd-8moQ%40mail.gmail.com
>

Re: Use pgBufferUsage for block reporting in analyze

[Anthonin Bonnefoy]

Thanks for having a look.

On Fri, May 10, 2024 at 12:40 PM Michael Paquier 
wrote:

> This needs some runtime check to make sure that the calculations
> are consistent before and after the fact (cannot do that now).
>
Yeah, testing this is also a bit painful as buffer usage of analyze is only
displayed in the logs during autoanalyze. While looking at this, I've
thought of additional changes that could make testing easier and improve
consistency with VACUUM VERBOSE:
- Have ANALYZE VERBOSE outputs the buffer usage stats
- Add Wal usage to ANALYZE VERBOSE

analyze verbose output would look like:
postgres=# analyze (verbose) pgbench_accounts ;
INFO:  analyzing "public.pgbench_accounts"
INFO:  "pgbench_accounts": scanned 1640 of 1640 pages, containing 10
live rows and 0 dead rows; 3 rows in sample, 10 estimated total rows
INFO:  analyze of table "postgres.public.pgbench_accounts"
avg read rate: 124.120 MB/s, avg write rate: 0.110 MB/s
buffer usage: 533 hits, 1128 reads, 1 dirtied
WAL usage: 12 records, 1 full page images, 5729 bytes
system usage: CPU: user: 0.06 s, system: 0.00 s, elapsed: 0.07 s

Perhaps this should say "read" rather than "miss" in the logs as the
> two read variables for the shared and local blocks are used?  For
> consistency, at least.
>
Sounds good.

That's not material for v17, only for v18.
>
 Definitely

I've split the patch in two parts
1: Removal of the vacuum specific variables, this is the same as the
initial patch.
2: Add buffer and wal usage to analyze verbose output + rename miss to
reads

Regards,
Anthonin


v2-0001-Use-pgBufferUsage-for-block-reporting-in-analyze.patch
Description: Binary data


v2-0002-Output-buffer-and-wal-usage-with-verbose-analyze.patch
Description: Binary data

Re: Conflict Detection and Resolution

[Nisha Moond]

On Mon, May 27, 2024 at 11:19 AM shveta malik  wrote:
>
> On Sat, May 25, 2024 at 2:39 AM Tomas Vondra
>  wrote:
> >
> > On 5/23/24 08:36, shveta malik wrote:
> > > Hello hackers,
> > >
> > > Please find the proposal for Conflict Detection and Resolution (CDR)
> > > for Logical replication.
> > >  > > below details.>
> > >
> > > Introduction
> > > 
> > > In case the node is subscribed to multiple providers, or when local
> > > writes happen on a subscriber, conflicts can arise for the incoming
> > > changes.  CDR is the mechanism to automatically detect and resolve
> > > these conflicts depending on the application and configurations.
> > > CDR is not applicable for the initial table sync. If locally, there
> > > exists conflicting data on the table, the table sync worker will fail.
> > > Please find the details on CDR in apply worker for INSERT, UPDATE and
> > > DELETE operations:
> > >
> >
> > Which architecture are you aiming for? Here you talk about multiple
> > providers, but the wiki page mentions active-active. I'm not sure how
> > much this matters, but it might.
>
> Currently, we are working for multi providers case but ideally it
> should work for active-active also. During further discussion and
> implementation phase, if we find that, there are cases which will not
> work in straight-forward way for active-active, then our primary focus
> will remain to first implement it for multiple providers architecture.
>
> >
> > Also, what kind of consistency you expect from this? Because none of
> > these simple conflict resolution methods can give you the regular
> > consistency models we're used to, AFAICS.
>
> Can you please explain a little bit more on this.
>
> >
> > > INSERT
> > > 
> > > To resolve INSERT conflict on subscriber, it is important to find out
> > > the conflicting row (if any) before we attempt an insertion. The
> > > indexes or search preference for the same will be:
> > > First check for replica identity (RI) index.
> > >   - if not found, check for the primary key (PK) index.
> > > - if not found, then check for unique indexes (individual ones or
> > > added by unique constraints)
> > >  - if unique index also not found, skip CDR
> > >
> > > Note: if no RI index, PK, or unique index is found but
> > > REPLICA_IDENTITY_FULL is defined, CDR will still be skipped.
> > > The reason being that even though a row can be identified with
> > > REPLICAT_IDENTITY_FULL, such tables are allowed to have duplicate
> > > rows. Hence, we should not go for conflict detection in such a case.
> > >
> >
> > It's not clear to me why would REPLICA_IDENTITY_FULL mean the table is
> > allowed to have duplicate values? It just means the upstream is sending
> > the whole original row, there can still be a PK/UNIQUE index on both the
> > publisher and subscriber.
>
> Yes, right. Sorry for confusion. I meant the same i.e. in absence of
> 'RI index, PK, or unique index', tables can have duplicates. So even
> in presence of Replica-identity (FULL in this case) but in absence of
> unique/primary index, CDR will be skipped for INSERT.
>
> >
> > > In case of replica identity ‘nothing’ and in absence of any suitable
> > > index (as defined above), CDR will be skipped for INSERT.
> > >
> > > Conflict Type:
> > > 
> > > insert_exists: A conflict is detected when the table has the same
> > > value for a key column as the new value in the incoming row.
> > >
> > > Conflict Resolution
> > > 
> > > a) latest_timestamp_wins:The change with later commit timestamp wins.
> > > b) earliest_timestamp_wins:   The change with earlier commit timestamp 
> > > wins.
> > > c) apply:   Always apply the remote change.
> > > d) skip:Remote change is skipped.
> > > e) error:   Error out on conflict. Replication is stopped, manual
> > > action is needed.
> > >
> >
> > Why not to have some support for user-defined conflict resolution
> > methods, allowing to do more complex stuff (e.g. merging the rows in
> > some way, perhaps even with datatype-specific behavior)?
>
> Initially, for the sake of simplicity, we are targeting to support
> built-in resolvers. But we have a plan to work on user-defined
> resolvers as well. We shall propose that separately.
>
> >
> > > The change will be converted to 'UPDATE' and applied if the decision
> > > is in favor of applying remote change.
> > >
> > > It is important to have commit timestamp info available on subscriber
> > > when latest_timestamp_wins or earliest_timestamp_wins method is chosen
> > > as resolution method.  Thus ‘track_commit_timestamp’ must be enabled
> > > on subscriber, in absence of which, configuring the said
> > > timestamp-based resolution methods will result in error.
> > >
> > > Note: If the user has chosen the latest or earliest_timestamp_wins,
> > > and the remote and local timestamps are the same, then it will go by
> > > system identifier. The change with a higher system identifier will
> > 

ON ERROR in json_query and the like

[Markus Winand]

Hi!

I’ve noticed two “surprising” (to me) behaviors related to
the “ON ERROR” clause of the new JSON query functions in 17beta1.

1. JSON parsing errors are not subject to ON ERROR
   Apparently, the functions expect JSONB so that a cast is implied
   when providing TEXT. However, the errors during that cast are
   not subject to the ON ERROR clause.

   17beta1=# SELECT JSON_QUERY('invalid', '$' NULL ON ERROR);
   ERROR:  invalid input syntax for type json
   DETAIL:  Token "invalid" is invalid.
   CONTEXT:  JSON data, line 1: invalid

   Oracle DB and Db2 (LUW) both return NULL in that case.

   I had a look on the list archive to see if that is intentional but
   frankly speaking these functions came a long way. In case it is
   intentional it might be worth adding a note to the docs.

2. EMPTY [ARRAY|OBJECT] ON ERROR implies ERROR ON EMPTY

   17beta1=# SELECT JSON_QUERY('[]', '$[*]' EMPTY ARRAY ON ERROR) a;
a
   
[]
   (1 row) 

   As NULL ON EMPTY is implied, it should give the same result as
   explicitly adding NULL ON EMPTY:

   17beta1=# SELECT JSON_QUERY('[]', '$[*]' NULL ON EMPTY EMPTY ARRAY ON ERROR) 
a;
a
   ---

   (1 row)

   Interestingly, Oracle DB gives the same (wrong) results. Db2 (LUW)
   on the other hand returns NULL for both queries.

   I don’t think that PostgreSQL should follow Oracle DB's suit here
   but again, in case this is intentional it should be made explicit
   in the docs.

-markus

Re: Parallel CREATE INDEX for GIN indexes

[Andy Fan]

Hi Tomas,

I have completed my first round of review, generally it looks good to
me, more testing need to be done in the next days. Here are some tiny
comments from my side, just FYI. 

1. Comments about GinBuildState.bs_leader looks not good for me. 
   
/*
 * bs_leader is only present when a parallel index build is performed, 
and
 * only in the leader process. (Actually, only the leader process has a
 * GinBuildState.)
 */
GinLeader  *bs_leader;

In the worker function _gin_parallel_build_main:
initGinState(&buildstate.ginstate, indexRel); is called, and the
following members in workers at least: buildstate.funcCtx,
buildstate.accum and so on.  So is the comment "only the leader process
has a GinBuildState" correct?

2. progress argument is not used?
_gin_parallel_scan_and_build(GinBuildState *state,
 GinShared *ginshared, 
Sharedsort *sharedsort,
 Relation heap, 
Relation index,
 int sortmem, bool 
progress)


3. In function tuplesort_begin_index_gin, comments about nKeys takes me
some time to think about why 1 is correct(rather than
IndexRelationGetNumberOfKeyAttributes) and what does the "only the index
key" means. 

base->nKeys = 1;/* Only the index key */

finally I think it is because gin index stores each attribute value into
an individual index entry for a multi-column index, so each index entry
has only 1 key.  So we can comment it as the following? 

"Gin Index stores the value of each attribute into different index entry
for mutli-column index, so each index entry has only 1 key all the
time." This probably makes it easier to understand.


4. GinBuffer: The comment "Similar purpose to BuildAccumulator, but much
simpler." makes me think why do we need a simpler but
similar structure, After some thoughts, they are similar at accumulating
TIDs only. GinBuffer is designed for "same key value" (hence
GinBufferCanAddKey). so IMO, the first comment is good enough and the 2 
comments introduce confuses for green hand and is potential to remove
it. 

/*
 * State used to combine accumulate TIDs from multiple GinTuples for the same
 * key value.
 *
 * XXX Similar purpose to BuildAccumulator, but much simpler.
 */
typedef struct GinBuffer


5. GinBuffer: ginMergeItemPointers always allocate new memory for the
new items and hence we have to pfree old memory each time. However it is
not necessary in some places, for example the new items can be appended
to Buffer->items (and this should be a common case). So could we
pre-allocate some spaces for items and reduce the number of pfree/palloc
and save some TID items copy in the desired case?

6. GinTuple.ItemPointerData first;  /* first TID in the array */

is ItemPointerData.ip_blkid good enough for its purpose?  If so, we can
save the memory for OffsetNumber for each GinTuple.

Item 5) and 6) needs some coding and testing. If it is OK to do, I'd
like to take it as an exercise in this area. (also including the item
1~4.)


-- 
Best Regards
Andy Fan

Re: why memoize is not used for correlated subquery

[Andy Fan]

> I imagined making this work by delaying the plan creation for
> subqueries until the same time as create_plan() for the outer query.

Do you mean sublinks rather than subqueries? if so, we can get another
benefit I want very much.

explain (costs off) select * from t1 where t1.a = 1
and exists (select 1 from t2 where  t2.a = t1.a and 
random() > 0);
  QUERY PLAN   
---
 Seq Scan on t1
   Filter: ((a = 1) AND EXISTS(SubPlan 1))
   SubPlan 1
 ->  Seq Scan on t2
   Filter: ((a = t1.a) AND (random() > '0'::double precision))

As for now, when we are planing the sublinks, we don't know t1.a = 1
which may lost some optimization chance.  Considering the t2.a is a
partition key of t2, this would yield some big improvement for planning
a big number of partitioned table.

-- 
Best Regards
Andy Fan

Re: ON ERROR in json_query and the like

[Pavel Stehule]

út 28. 5. 2024 v 11:29 odesílatel Markus Winand 
napsal:

> Hi!
>
> I’ve noticed two “surprising” (to me) behaviors related to
> the “ON ERROR” clause of the new JSON query functions in 17beta1.
>
> 1. JSON parsing errors are not subject to ON ERROR
>Apparently, the functions expect JSONB so that a cast is implied
>when providing TEXT. However, the errors during that cast are
>not subject to the ON ERROR clause.
>
>17beta1=# SELECT JSON_QUERY('invalid', '$' NULL ON ERROR);
>ERROR:  invalid input syntax for type json
>DETAIL:  Token "invalid" is invalid.
>CONTEXT:  JSON data, line 1: invalid
>
>Oracle DB and Db2 (LUW) both return NULL in that case.
>
>I had a look on the list archive to see if that is intentional but
>frankly speaking these functions came a long way. In case it is
>intentional it might be worth adding a note to the docs.
>

I remember a talk about this subject years ago. Originally the JSON_QUERY
was designed in similar like Oracle, and casting to jsonb was done inside.
If I remember this behave depends on the fact, so old SQL/JSON has not json
type and it was based just on processing of plain text. But Postgres has
JSON, and JSONB and then was more logical to use these types. And because
the JSON_QUERY uses these types, and the casting is done before the
execution of the function, then the clause ON ERROR cannot be handled.
Moreover, until soft errors Postgres didn't allow handling input errors in
common functions.

I think so this difference should be mentioned in documentation.

Regards

Pavel


> 2. EMPTY [ARRAY|OBJECT] ON ERROR implies ERROR ON EMPTY
>
>17beta1=# SELECT JSON_QUERY('[]', '$[*]' EMPTY ARRAY ON ERROR) a;
> a
>
> []
>(1 row)
>
>As NULL ON EMPTY is implied, it should give the same result as
>explicitly adding NULL ON EMPTY:
>
>17beta1=# SELECT JSON_QUERY('[]', '$[*]' NULL ON EMPTY EMPTY ARRAY ON
> ERROR) a;
> a
>---
>
>(1 row)
>
>Interestingly, Oracle DB gives the same (wrong) results. Db2 (LUW)
>on the other hand returns NULL for both queries.
>
>I don’t think that PostgreSQL should follow Oracle DB's suit here
>but again, in case this is intentional it should be made explicit
>in the docs.
>
> -markus
>
>
>
>

Re:[PATCH]A minor improvement to the error-report in SimpleLruWriteAll()

[Long Song]

Hi,
Actually, I still wonder why only the error message
of the last failure to close the file was recorded.
For this unusual situation, it is acceptable to
record all failure information without causing
too much logging.
Was it designed that way on purpose?




At 2024-05-25 17:29:00, "Long Song"  wrote:



Hi hackers,
When I read the code, I noticed that in SimpleLruWriteAll(), only the last 
error is 
recorded when the file fails to close. Like the following,
```void
SimpleLruWriteAll(SlruCtl ctl, bool allow_redirtied)
{
SlruShared  shared = ctl->shared;
SlruWriteAllData fdata;
int64   pageno = 0;
int prevbank = SlotGetBankNumber(0);
boolok;
...
/*
 * Now close any files that were open
 */
ok = true;
for (int i = 0; i < fdata.num_files; i++)
{
if (CloseTransientFile(fdata.fd[i]) != 0)
{
slru_errcause = SLRU_CLOSE_FAILED;
slru_errno = errno;
pageno = fdata.segno[i] * SLRU_PAGES_PER_SEGMENT;
ok = false;
}
}
if (!ok)
SlruReportIOError(ctl, pageno, InvalidTransactionId);
```
// Here, SlruReportIOError() is called only once, meaning that the last error 
message is
 recorded. In my opinion, since failure to close a file is not common, logging 
an error 
message every time a failure occurs will not result in much log growth, but 
detailed error 
logging will help in most cases.

So, I changed the code to move the call to SlruReportIOError() inside the while 
loop.

Attached is the patch, I hope it can help.

Re: pgsql: Add more SQL/JSON constructor functions

[David G. Johnston]

On Monday, May 27, 2024, Alvaro Herrera  wrote:

> On 2024-May-27, Alvaro Herrera wrote:
>
> > > JSON_SERIALIZE()
>
> I just noticed this behavior, which looks like a bug to me:
>
> select json_serialize('{"a":1, "a":2}' returning varchar(5));
>  json_serialize
> 
>  {"a":
>
> I think this function should throw an error if the destination type
> doesn't have room for the output json.  Otherwise, what good is the
> serialization function?
>
>
It’s not a self-evident bug given that this is exactly how casting data to
varchar(n) behaves as directed by the SQL Standard.

I'd probably leave the internal consistency and take the opportunity to
educate the reader that text is the preferred type in PostgreSQL and,
especially here, there is little good reason to use anything else.

David J.

Re: commitfest.postgresql.org is no longer fit for purpose

[James Coleman]

On Thu, May 16, 2024 at 4:00 PM Joe Conway  wrote:
>
> On 5/16/24 17:36, Jacob Champion wrote:
> > On Thu, May 16, 2024 at 2:29 PM Joe Conway  wrote:
> >> If no one, including the author (new or otherwise) is interested in
> >> shepherding a particular patch, what chance does it have of ever getting
> >> committed?
> >
> > That's a very different thing from what I think will actually happen, which 
> > is
> >
> > - new author posts patch
> > - community member says "use commitfest!"
>
> Here is where we should point them at something that explains the care
> and feeding requirements to successfully grow a patch into a commit.
>
> > - new author registers patch
> > - no one reviews it
> > - patch gets automatically booted
>
> Part of the care and feeding instructions should be a warning regarding
> what happens if you are unsuccessful in the first CF and still want to
> see it through.
>
> > - community member says "register it again!"
> > - new author says ಠ_ಠ
>
> As long as this is not a surprise ending, I don't see the issue.

I've experienced this in another large open-source project that runs
on Github, not mailing lists, and here's how it goes:

1. I open a PR with a small bugfix (test case included).
2. PR is completely ignored by committers (presumably because they all
mostly work on their own projects they're getting paid to do).
3. <3 months goes by>
4. I may get a comment with "please rebase!", or, more frequently, a
bot closes the issue.

That cycle is _infuriating_ as a contributor. As much as I don't like
to hear "we're not doing this", I'd far prefer to have that outcome
then some automated process closing out my submission without my input
when, as far as I can tell, the real problem is not my lack of
activity by the required reviewers simply not looking at it.

So I'm genuinely confused by you say "As long as this is not a
surprise ending, I don't see the issue.". Perhaps we're imagining
something different here reading between the lines?

Regards,
James Coleman

Re: commitfest.postgresql.org is no longer fit for purpose

[James Coleman]

On Fri, May 17, 2024 at 9:59 AM Robert Haas  wrote:
>
> On Fri, May 17, 2024 at 11:05 AM Tom Lane  wrote:
> > > We already have gone back to that model. We just haven't admitted it
> > > yet. And we're never going to get out of it until we find a way to get
> > > the contents of the CommitFest application down to a more reasonable
> > > size and level of complexity. There's just no way everyone's up for
> > > that level of pain. I'm not sure not up for that level of pain.
> >
> > Yeah, we clearly need to get the patch list to a point of
> > manageability, but I don't agree that abandoning time-boxed CFs
> > will improve anything.
>
> I'm not sure. Suppose we plotted commits generally, or commits of
> non-committer patches, or reviews on-list, vs. time. Would we see any
> uptick in activity during CommitFests? Would it vary by committer? I
> don't know. I bet the difference wouldn't be as much as Tom Lane would
> like to see. Realistically, we can't manage how contributors spend
> their time all that much, and trying to do so is largely tilting at
> windmills.
>
> To me, the value of time-based CommitFests is as a vehicle to ensure
> freshness, or cleanup, or whatever word you want to do. If you just
> make a list of things that need attention and keep incrementally
> updating it, eventually for various reasons that list no longer
> reflects your current list of priorities. At some point, you have to
> throw it out and make a new list, or at least that's what always
> happens to me. We've fallen into a system where the default treatment
> of a patch is to be carried over to the next CommitFest and CfMs are
> expected to exert effort to keep patches from getting that default
> treatment when they shouldn't. But that does not scale. We need a
> system where things drop off the list unless somebody makes an effort
> to keep them on the list, and the easiest way to do that is to
> periodically make a *fresh* list that *doesn't* just clone some
> previous list.
>
> I realize that many people here are (rightly!) concerned with
> burdening patch authors with more steps that they have to follow. But
> the current system is serving new patch authors very poorly. If they
> get attention, it's much more likely to be because somebody saw their
> email and wrote back than it is to be because somebody went through
> the CommitFest and found their entry and was like "oh, I should review
> this". Honestly, if we get to a situation where a patch author is sad
> because they have to click a link every 2 months to say "yeah, I'm
> still here, please review my patch," we've already lost the game. That
> person isn't sad because we asked them to click a link. They're sad
> it's already been N * 2 months and nothing has happened.

Yes, this is exactly right.

This is an off-the-wall idea, but what if the inverse is actually what
we need? Suppose there's been a decent amount of activity previously
on the thread, but no new patch version or CF app activity (e.g.,
status changes moving it forward) or maybe even just the emails died
off: perhaps that should trigger a question to the author to see what
they want the status to be -- i.e., "is this still 'needs review', or
is it really 'waiting on author' or 'not my priority right now'?"

It seems possible to me that that would actually remove a lot of the
patches from the current CF when a author simply hasn't had time to
respond yet (I know this is the case for me because the time I have to
work on patches fluctuates significantly), but it might also serve to
highlight patches that simply haven't had any review at all.

I'd like to add a feature to the CF app that shows me my current
patches by status, and I'd also like to have the option to have the CF
app notify me when someone changes the status (I've noticed before
that often a status gets changed without notification on list, and
then I get surprised months later when it's stuck in "waiting on
author"). Do either/both of those seem reasonable to add?

Regards,
James Coleman

Re: Add last_commit_lsn to pg_stat_database

[James Coleman]

On Thu, Mar 7, 2024 at 10:30 AM Heikki Linnakangas  wrote:
>
> > I've previously noted in "Add last commit LSN to
> > pg_last_committed_xact()" [1] that it's not possible to monitor how
> > many bytes of WAL behind a logical replication slot is (computing such
> > is obviously trivial for physical slots) because the slot doesn't need
> > to replicate beyond the last commit. In some cases it's possible for
> > the current WAL location to be far beyond the last commit. A few
> > examples:
> >
> > - An idle server with checkout_timeout at a lower value (so lots of
> > empty WAL advance).
> > - Very large transactions: particularly insidious because committing a
> > 1 GB transaction after a small transaction may show almost zero time
> > lag even though quite a bit of data needs to be processed and sent
> > over the wire (so time to replay is significantly different from
> > current lag).
> > - A cluster with multiple databases complicates matters further,
> > because while physical replication is cluster-wide, the LSNs that
> > matter for logical replication are database specific.
> >
> >
> > Since we don't expose the most recent commit's LSN there's no way to
> > say "the WAL is currently 1250, the last commit happened at 1000, the
> > slot has flushed up to 800, therefore there are at most 200 bytes
> > replication needs to read through to catch up.
>
> I'm not sure I fully understand the problem. What are you doing
> currently to measure the lag? If you look at pg_replication_slots today,
> confirmed_flush_lsn advances also when you do pg_switch_wal(), so
> looking at the diff between confirmed_flush_lsn and pg_current_wal_lsn()
> works, no?

No, it's not that simple because of the "large, uncommitted
transaction" case I noted in the OP. Suppose I have a batch system,
and a job in that system has written 1 GB of data but not yet
committed (or rolled back). In this case confirmed_flush_lsn cannot
advance, correct?

And so having a "last commit LSN" allows me to know how far back the
"last possibly replicatable write"

> And on the other hand, even if you expose the database's last commit
> LSN, you can have an publication that includes only a subset of tables.
> Or commits that don't write to any table at all. So I'm not sure why the
> database's last commit LSN is relevant. Getting the last LSN that did
> something that needs to be replicated through the publication might be
> useful, but that's not what what this patch does.

I think that's fine,  because as you noted earlier the
confirmed_flush_lsn could advance beyond that point anyway (if there's
nothing to replicate), but in the case where:

1. confirmed_flush_lsn is not advancing, and
2. last_commit_lsn is not advancing, and
3. pg_current_wal_lsn() has advanced a lot

then you can probably infer that there's a large amount of data that
simply cannot be completed by the subscriber, and so there's no "real"
delay. It also gives you an idea of how much data you will need to
churn through (even if not replicated) once the transaction commits.

Certainly understanding the data here will be simplest in the case
where 1.) there's a single database and 2.) all tables are in the
replication set, but I don't think the value is limited to that
situation either.

Regards,
James Coleman

small fix for llvm build

[Peter Eisentraut]

I'm getting build failures when building with meson and llvm enabled, 
like this:


[1/112] Generating src/backend/jit/llvm/llvmjit_types.bc with a custom 
command

FAILED: src/backend/jit/llvm/llvmjit_types.bc
/usr/local/bin/ccache /usr/local/Cellar/llvm/18.1.6/bin/clang -c -o 
src/backend/jit/llvm/llvmjit_types.bc 
../src/backend/jit/llvm/llvmjit_types.c -flto=thin -emit-llvm -MD -MQ 
src/backend/jit/llvm/llvmjit_types.bc -MF 
src/backend/jit/llvm/llvmjit_types.c.bc.d -O2 -Wno-ignored-attributes 
-Wno-empty-body -fno-strict-aliasing -fwrapv -I./src/include 
-I./src/backend/utils/misc -I../src/include

In file included from ../src/backend/jit/llvm/llvmjit_types.c:27:
In file included from ../src/include/postgres.h:45:
../src/include/c.h:75:10: fatal error: 'libintl.h' file not found
   75 | #include 
  |  ^~~
1 error generated.


The reason is that libintl.h is at /usr/local/include/libintl.h, but 
that is not in the include path for this command.  I have 
-I/usr/local/include in CPPFLAGS in the environment, which is why the 
normal compilation commands pick it up, but this is not used by this 
custom command.


Wit this small patch I can make it work:

diff --git a/src/backend/jit/llvm/meson.build 
b/src/backend/jit/llvm/meson.build

index 41c759f73c5..4a4232661ba 100644
--- a/src/backend/jit/llvm/meson.build
+++ b/src/backend/jit/llvm/meson.build
@@ -63,6 +63,7 @@ bitcode_cflags = ['-fno-strict-aliasing', '-fwrapv']
 if llvm.version().version_compare('=15.0')
   bitcode_cflags += ['-Xclang', '-no-opaque-pointers']
 endif
+bitcode_cflags += get_option('c_args')
 bitcode_cflags += cppflags

 # XXX: Worth improving on the logic to find directories here


Is that correct?

Re: Schema variables - new implementation for Postgres 15

[Pavel Stehule]

Hi

so 25. 5. 2024 v 3:29 odesílatel Tom Lane  napsal:

> Pavel Stehule  writes:
> > we can introduce special safe mode started by
> > set enable_direct_variable_read to off;
> > and allowing access to variables only by usage dedicated function
> > (supported by parser) named like variable or pg_variable
>
> Didn't we learn twenty years ago that GUCs that change query
> semantics are an awful idea?  Pick a single access method
> for these things and stick to it.
>

I propose another variants. First we can introduce pseudo function VAR( ).
The argument should be session variables. The name of this function can be
pgvar, globvar, ... We can talk about good name, it should not be too long,
but it is not important now. The VAR() function will be pseudo function
like COALESCE, so we can easily to set correct result type.

I see possible variants

1. for any read of session variable, the VAR function should be used
(everywhere), the write is not problem, there is not risk of collisions.
When VAR() function will be required everywhere, then the name should be
shorter.

SELECT * FROM tab WHERE id = VAR(stehule.myvar);
SELECT VAR(okbob.myvar);

2. the usage of VAR() function should be required, when query has FROM
clause, and then there is in risk of collisions. Without it, then the VAR()
function can be optional (it is modification of Wolfgang or Alvaro
proposals). I prefer this syntax before mentioning in FROM clause, just I
think so it is less confusing, and FROM clause should be used for
relations, and not for variables.

SELECT * FROM tab WHERE id = VAR(okbob.myvar)
SELECT okbob.myvar;

3. Outside PL the VAR() function will be required, inside PL the VAR
function can be optional (and we can throw an exception) when we found
collision like now

What do you think about this proposal? And if you can accept it, what
version?

I think so implementation of any proposed variant should be easy. I can add
extra check to plpgsql_check if the argument of VAR() function is in
possible collision with other identifiers in query, but for proposed
variants it is just in nice to have category

Regards

Pavel



>
> regards, tom lane
>

Re: Add last_commit_lsn to pg_stat_database

[James Coleman]

On Mon, Feb 19, 2024 at 3:56 PM Michael Paquier  wrote:
>
> On Mon, Feb 19, 2024 at 10:26:43AM +0100, Tomas Vondra wrote:
> > On 2/19/24 07:57, Michael Paquier wrote:
> > > On Sun, Feb 18, 2024 at 02:28:06AM +0100, Tomas Vondra wrote:
> >>> 1) Do we really need to modify RecordTransactionCommitPrepared and
> >>> XactLogCommitRecord to return the LSN of the commit record? Can't we
> >>> just use XactLastRecEnd? It's good enough for advancing
> >>> replorigin_session_origin_lsn, why wouldn't it be good enough for this?
> >>
> >> Or XactLastCommitEnd?
> >
> > But that's not set in RecordTransactionCommitPrepared (or twophase.c in
> > general), and the patch seems to need that.
>
> Hmm.  Okay.
>
> > > The changes in AtEOXact_PgStat() are not really
> > > attractive for what's a static variable in all the backends.
> >
> > I'm sorry, I'm not sure what "changes not being attractive" means :-(
>
> It just means that I am not much a fan of the signature changes done
> for RecordTransactionCommit() and AtEOXact_PgStat_Database(), adding a
> dependency to a specify LSN.  Your suggestion to switching to
> XactLastRecEnd should avoid that.

Attached is an updated patch that uses XactLastCommitEnd and therefore
avoids all of those signature changes.

We can't use XactLastCommitEnd because it gets set to 0 immediately
after RecordTransactionCommit() sets XactLastCommitEnd to its value.

I added a test for two-phase commit, as well, and in so doing I
discovered something that I found curious: when I do "COMMIT PREPARED
't1'", not only does RecordTransactionCommitPrepared() get called, but
eventually RecordTransactionCommit() is called as well before the
command returns (despite the comments for those functions describing
them as two equivalents you need to change at the same time).

So it appears that we don't need to set XactLastCommitEnd in
RecordTransactionCommitPrepared() for this to work, and, indeed,
adding some logging to verify, the value of XactLastRecEnd we'd use to
update XactLastCommitEnd is the same at the end of both of those
functions during COMMIT PREPARED.

I'd like to have someone weigh in on whether relying on
RecordTransactionCommit() being called during COMMIT PREPARED is
correct or not (if perchance there are non-obvious reasons why we
shouldn't).

Regards,
James Coleman


v4-0001-Add-last-commit-s-LSN-to-pg_stat_database.patch
Description: Binary data

Re: errors building on windows using meson

[Imran Zaheer]

 Hi

I was facing the same error when using active state perl for compiling
postgres with meson on windows. I used active state perl because it was
working fine for pg compilations until pg-16.

Using choco strawberry perl solved my problem.

Thanks

Imran Zaheer

On Wed, 29 May 2024 at 00:22, Dave Cramer  wrote:

>
> On Thu, 7 Dec 2023 at 14:34, Andres Freund  wrote:
>
>> Hi,
>>
>> On 2023-12-07 14:16:52 -0500, Dave Cramer wrote:
>> > On Thu, 7 Dec 2023 at 13:53, Andres Freund  wrote:
>> >
>> > > Hi,
>> > >
>> > > On 2023-12-07 12:54:27 -0500, Dave Cramer wrote:
>> > > > state-exec: run failed: cannot create new executor meta: cannot get
>> > > > matching bin by path: no matching binary by path
>> > > >
>> > >
>> "C:\\Users\\Administrator\\AppData\\Local\\activestate\\cache\\b9117b06\\exec\\perl.EXE"
>> > > > state-exec: Not user serviceable; Please contact support for
>> assistance.
>> > > >
>> > > > anyone seen this or have a fix ?
>> > >
>> > > I've not seen that before. Please provide a bit more detail. Compiler,
>> > > building with ninja or msbuild/visual studio, when exactly you're
>> > > encountering
>> > > the issue, ...
>> > >
>> > > Windows Server 2019
>> > VS 2019
>> > building with ninja
>>
>> I don't think this is sufficient detail to provide you with advice / fix
>> problems / whatnot. Please provide complete logs of configuring and
>> building.
>>
>
> I built perl from source and it worked.
>
> Dave
>
>
>
>>
>> - Andres
>>
>

Avoid an odd undefined behavior with memcmp (src/bin/pg_rewind/pg_rewind.c)

[Ranier Vilela]

Hi.

The function *perform_rewind* has an odd undefined behavior.
The function memcmp/ ,
compares bytes to bytes.

IMO, I think that pg_rewind can have a security issue,
if two files are exactly the same, they are considered different.
Because use of structs with padding values is unspecified.

Fix by explicitly initializing with memset to avoid this.

best regards,
Ranier Vilela


avoid-undefined-compares-two-structs-with-padding-bytes-pg_rewind.patch
Description: Binary data

Windows: openssl & gssapi dislike each other

[Dave Page]

I recently noticed that at least with PostgreSQL 16, it is not possible to
build using MSVC if both OpenSSL and gssapi (MIT Kerberos) are enabled.
Both work if the other isn't included though..

I briefly tried to test with PG17 to see if it has the same issue, but it
seems like gssapi has the same problem I recently found with zlib (
https://www.postgresql.org/message-id/CA%2BOCxozrPZx57ue8rmhq6CD1Jic5uqKh80%3DvTpZurSKESn-dkw%40mail.gmail.com
).

I've yet to find time to look into this - reporting anyway rather than
sitting on it until I get round to it...

Build FAILED.

"C:\Users\dpage\Downloads\postgresql-16.3\pgsql.sln" (default target) (1) ->
"C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj" (default
target) (9) ->
(ClCompile target) ->
  C:\build64\openssl\include\openssl\x509v3.h(201,1): warning C4228:
nonstandard extension used: qualifiers after comma in declarator list are
ignored [C:\User
s\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(227,1): warning C4228:
nonstandard extension used: qualifiers after comma in declarator list are
ignored [C:\User
s\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(527,1): warning C4228:
nonstandard extension used: qualifiers after comma in declarator list are
ignored [C:\User
s\dpage\Downloads\postgresql-16.3\postgres.vcxproj]

C:\Users\dpage\Downloads\postgresql-16.3\src\backend\utils\sort\tuplesort.c(2000,1):
warning C4724: potential mod by 0 [C:\Users\dpage\Downloads\postgresql-1
6.3\postgres.vcxproj]


"C:\Users\dpage\Downloads\postgresql-16.3\pgsql.sln" (default target) (1) ->
"C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj" (default
target) (9) ->
(ClCompile target) ->
  C:\build64\openssl\include\openssl\x509v3.h(181,9): error C2059: syntax
error: '(' [C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(188,9): error C2059: syntax
error: ''
[C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(193,5): error C2059: syntax
error: '}' [C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(194,1): error C2059: syntax
error: '}' [C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(198,5): error C2061: syntax
error: identifier 'GENERAL_NAME'
[C:\Users\dpage\Downloads\postgresql-16.3\postgres.v
cxproj]
  C:\build64\openssl\include\openssl\x509v3.h(199,1): error C2059: syntax
error: '}' [C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2143: syntax
error: missing ')' before '*'
[C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxp
roj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2143: syntax
error: missing '{' before '*'
[C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxp
roj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2143: syntax
error: missing ';' before '*'
[C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxp
roj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2059: syntax
error: ')' [C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2373: 'a':
redefinition; different type modifiers
[C:\Users\dpage\Downloads\postgresql-16.3\postgr
es.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2054: expected
'(' to follow 'ptr'
[C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2146: syntax
error: missing ')' before identifier 'cmp'
[C:\Users\dpage\Downloads\postgresql-16.3\
postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2061: syntax
error: identifier 'cmp'
[C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2059: syntax
error: ';' [C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2449: found
'{' at file scope (missing function header?)
[C:\Users\dpage\Downloads\postgresql-16.3
\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2059: syntax
error: '}' [C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2146: syntax
error: missing ')' before identifier 'fr'
[C:\Users\dpage\Downloads\postgresql-16.3\p
ostgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(201,1): error C2061: syntax
error: identifier 'fr'
[C:\Users\dpage\Downloads\postgresql-16.3\postgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(227,1): error C2236:
unexpected token 'struct'. Did you forget a ';'?
[C:\Users\dpage\Downloads\postgresql-16.3\p
ostgres.vcxproj]
  C:\build64\openssl\include\openssl\x509v3.h(227,1): erro

Re: Need clarification on compilation errors in PG 16.2

[Tom Lane]

Pradeep Kumar  writes:
> Yes it was defined in "pg_config.h"
> /* Define to 1 if you have the `memset_s' function. */
> #define HAVE_MEMSET_S 1

That's correct for recent versions of macOS.  I see you are
building against a recent SDK:

/Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/string.h

but I wonder if maybe the actual OS version is back-rev?

regards, tom lane

Re: Need clarification on compilation errors in PG 16.2

[Daniel Gustafsson]

> On 28 May 2024, at 22:51, Tom Lane  wrote:
> 
> Pradeep Kumar  writes:
>> Yes it was defined in "pg_config.h"
>> /* Define to 1 if you have the `memset_s' function. */
>> #define HAVE_MEMSET_S 1
> 
> That's correct for recent versions of macOS.  I see you are
> building against a recent SDK:
> 
> /Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/string.h
> 
> but I wonder if maybe the actual OS version is back-rev?

Skimming the releases on https://opensource.apple.com/releases/ it seems that
memset_s has been available since Mavericks (10.9) AFAICT.

--
Daniel Gustafsson

Re: meson vs windows perl

[Andres Freund]

Hi,

On 2024-04-05 16:12:12 -0400, Andrew Dunstan wrote:
> OK, this has been fixed and checked. The attached is what I propose.

The perl command is pretty hard to read. What about using python's shlex
module instead? Rough draft attached.  Still not very pretty, but seems easier
to read?

It'd be even better if we could just get perl to print out the flags in an
easier to parse way, but I couldn't immediately see a way.

Greetings,

Andres Freund
diff --git i/meson.build w/meson.build
index d6401fb8e30..191a051defb 100644
--- i/meson.build
+++ w/meson.build
@@ -997,13 +997,20 @@ if not perlopt.disabled()
 undesired = run_command(perl_conf_cmd, 'ccdlflags', check: true).stdout().split()
 undesired += run_command(perl_conf_cmd, 'ldflags', check: true).stdout().split()
 
+ldopts_split = run_command(python, '-c', '''
+import shlex
+import sys
+
+print('\n'.join(shlex.split(sys.argv[1])), end='')
+''',
+ ldopts, check: true).stdout().split('\n')
+
 perl_ldopts = []
-foreach ldopt : ldopts.split(' ')
-  if ldopt == '' or ldopt in undesired
+foreach ldopt : ldopts_split
+  if ldopt in undesired
 continue
   endif
-
-  perl_ldopts += ldopt.strip('"')
+  perl_ldopts += ldopt
 endforeach
 
 message('LDFLAGS recommended by perl: "@0@"'.format(ldopts))

Re: In-placre persistance change of a relation

[Jelte Fennema-Nio]

On Fri, 24 May 2024 at 00:09, Kyotaro Horiguchi  wrote:
> Along with rebasing, I changed the interface of XLogFsyncFile() to
> return a boolean instead of an error message.

Two notes after looking at this quickly during the advanced patch
feedback session:

1. I would maybe split 0003 into two separate patches. One to make SET
UNLOGGED fast, which seems quite easy to do because no WAL is needed.
And then a follow up to make SET LOGGED fast, which does all the
XLOG_FPI stuff.
2. When wal_level = minital, still some WAL logging is needed. The
pages that were changed since the last still need to be made available
for crash recovery.

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Hannu Krosing]

Hi Daniel,

pg_upgrade is just one important user of pg_dump which is the one that
generates REVOKE for a non-existent role.

We should definitely also fix pg_dump, likely just checking that the
role exists when generating REVOKE commands (may be a good practice
for other cases too so instead of casting to ::regrole  do the actual
join)


## here is the fix for pg_dump

While flying to Vancouver I looked around in pg_dump code, and it
looks like the easiest way to mitigate the dangling pg_init_priv
entries is to replace the query in pg_dump with one that filters out
invalid entries

The current query is at line 9336:

/* Fetch initial-privileges data */
if (fout->remoteVersion >= 90600)
{
printfPQExpBuffer(query,
  "SELECT objoid, classoid, objsubid, privtype, initprivs "
  "FROM pg_init_privs");

res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);


And we need the same but filtering out invalid aclitems from initprivs
something like this

WITH q AS (
  SELECT objoid, classoid, objsubid, privtype, unnest(initprivs) AS
initpriv FROM saved_init_privs
)
SELECT objoid, classoid, objsubid, privtype, array_agg(initpriv) as initprivs
  FROM q
 WHERE is_valid_value_for_type(initpriv::text, 'aclitem')
 GROUP BY 1,2,3,4;

### The proposed re[placement query:

Unfortunately we do not have an existing
is_this_a_valid_value_for_type(value text, type text, OUT res boolean)
function, so for a read-only workaround the following seems to work:

Here I first collect the initprivs array elements which fail the
conversion to text and back into an array and store it in GUC
pg_dump.bad_aclitems

Then I use this stored list to filter out the bad ones in the actual query.

DO $$
DECLARE
  aclitem_text text;
  bad_aclitems text[] = '{}';
BEGIN
  FOR aclitem_text IN
SELECT DISTINCT unnest(initprivs)::text FROM pg_init_privs
  LOOP
BEGIN /* try to convert back to aclitem */
  PERFORM aclitem_text::aclitem;
EXCEPTION WHEN OTHERS THEN /* collect bad aclitems */
  bad_aclitems := bad_aclitems || ARRAY[aclitem_text];
END;
  END LOOP;
  IF bad_aclitems != '{}' THEN
RAISE WARNING 'Ignoring bad aclitems "%" in pg_init_privs', bad_aclitems;
  END IF;
  PERFORM set_config('pg_dump.bad_aclitems', bad_aclitems::text,
false); -- true for trx-local
END;
$$;
WITH q AS (
  SELECT objoid, classoid, objsubid, privtype, unnest(initprivs) AS
initpriv FROM pg_init_privs
)
SELECT objoid, classoid, objsubid, privtype, array_agg(initpriv) AS initprivs
  FROM q
 WHERE NOT initpriv::text = ANY
(current_setting('pg_dump.bad_aclitems')::text[])
 GROUP BY 1,2,3,4;

--
Hannu

On Sun, May 26, 2024 at 11:27 PM Daniel Gustafsson  wrote:
>
> > On 26 May 2024, at 23:25, Tom Lane  wrote:
> >
> > Hannu Krosing  writes:
> >> Attached is a minimal patch to allow missing roles in REVOKE command
> >
> > FTR, I think this is a very bad idea.
>
> Agreed, this is papering over a bug.  If we are worried about pg_upgrade it
> would be better to add a check to pg_upgrade which detects this case and
> advices the user how to deal with it.
>
> --
> Daniel Gustafsson
>

Re: pgsql: Add more SQL/JSON constructor functions

[Amit Langote]

Hi Alvaro,

On Mon, May 27, 2024 at 7:10 PM Alvaro Herrera  wrote:
>
> On 2024-May-27, Alvaro Herrera wrote:
>
> > > JSON_SERIALIZE()
>
> I just noticed this behavior, which looks like a bug to me:
>
> select json_serialize('{"a":1, "a":2}' returning varchar(5));
>  json_serialize
> 
>  {"a":
>
> I think this function should throw an error if the destination type
> doesn't have room for the output json.  Otherwise, what good is the
> serialization function?

I remember using the reasoning mentioned by David G when testing
json_query() et al with varchar(n), so you get:

select json_query('{"a":1, "a":2}', '$' returning varchar(5));
 json_query

 {"a":
(1 row)

which is the same as:

select '{"a":1, "a":2}'::varchar(5);
 varchar
-
 {"a":
(1 row)

Also,

select json_value('{"a":"abcdef"}', '$.a' returning varchar(5) error on error);
 json_value

 abcde
(1 row)

This behavior comes from using COERCE_EXPLICIT_CAST when creating the
coercion expression to convert json_*() functions' argument to the
RETURNING type.

-- 
Thanks, Amit Langote

Re: Reducing memory consumed by RestrictInfo list translations in partitionwise join planning

[Ashutosh Bapat]

On Mon, Feb 19, 2024 at 5:17 AM Ashutosh Bapat 
wrote:

> On Mon, Feb 19, 2024 at 4:35 AM Tomas Vondra
>  wrote:
> >
> > Hi,
> >
> > After taking a look at the patch optimizing SpecialJoinInfo allocations,
> > I decided to take a quick look at this one too. I don't have any
> > specific comments on the code yet, but it seems quite a bit more complex
> > than the other patch ... it's introducing a HTAB into the optimizer,
> > surely that has costs too.
>
> Thanks for looking into this too.
>
> >
> > I started by doing the same test as with the other patch, comparing
> > master to the two patches (applied independently and both). And I got
> > about this (in MB):
> >
> >   tablesmaster sjinforinfo  both
> >   ---
> >237 36   3433
> >3   138129  122   113
> >4   421376  363   318
> >5  1495   1254 1172   931
> >
> > Unlike the SpecialJoinInfo patch, I haven't seen any reduction in
> > planning time for this one.
>
> Yeah. That agreed with my observation as well.
>
> >
> > The reduction in allocated memory is nice. I wonder what's allocating
> > the remaining memory, and we'd have to do to reduce it further.
>
> Please see reply to SpecialJoinInfo thread. Other that the current
> patches, we require memory efficient Relids implementation. I have
> shared some ideas in the slides I shared in the other thread, but
> haven't spent time experimenting with any ideas there.
>
> >
> > However, this is a somewhat extreme example - it's joining 5 tables,
> > each with 1000 partitions, using a partition-wise join. It reduces the
> > amount of memory, but the planning time is still quite high (and
> > essentially the same as without the patch). So it's not like it'd make
> > them significantly more practical ... do we have any other ideas/plans
> > how to improve that?
>
> Yuya has been working on reducing planning time [1]. Has some
> significant improvements in that area based on my experiments. But
> those patches are complex and still WIP.
>
> >
> > AFAIK we don't expect this to improve "regular" cases with modest number
> > of tables / partitions, etc. But could it make them slower in some case?
> >
>
> AFAIR, my experiments did not show any degradation in regular cases
> with modest number of tables/partitions. The variation in planning
> time was with the usual planning time variations.
>

Documenting some comments from todays' patch review session
1. Instead of a nested hash table, it might be better to use a flat hash
table to save more memory.
2. new comm_rinfo member in RestrictInfo may have problems when copying
RestrictInfo or translating it. Instead commuted versions may be tracked
outside RestrictInfo

Combining the above two, it feels like we need a single hash table with
(commuted, rinfo_serial, relids) as key to store all the translations of a
RestrictInfo.

-- 
Best Wishes,
Ashutosh Bapat

Re: Improving the latch handling between logical replication launcher and worker processes.

[Peter Smith]

On Thu, Apr 25, 2024 at 6:59 PM vignesh C  wrote:
>
...
> a) Introduce a new latch to handle worker attach and exit.

IIUC there is normally only the “shared” latch or a “process local”
latch. i.e. AFAICT is is quite uncommon to invent a new latches like
option a is proposing. I did not see any examples of making new
latches (e.g. MyLatchA, MyLatchB, MyLatchC) in the PostgreSQL source
other than that ‘recoveryWakeupLatch’ mentioned above by Hou-san. So
this option could be OK, but OTOH since there is hardly any precedent
maybe that should be taken as an indication to avoid doing it this way
(???)

> b) Add a new GUC launcher_retry_time which gives more flexibility to
> users as suggested by Amit at [1].

I'm not sure that introducing a new GUC is a good option because this
seems a rare problem to have -- so it will be hard to tune since it
will be difficult to know you even have this problem and then
difficult to know that it is fixed. Anyway. this made me consider more
what the WaitLatch timeout value should be. Here are some thoughts:

Idea 1)

I was wondering where did that DEFAULT_NAPTIME_PER_CYCLE value of 180
seconds come from or was that just a made-up number? AFAICT it just
came into existence in the first pub/sub commit [1] but there is no
explanation for why 180s was chosen. Anyway, I assume a low value
(5s?) would be bad because it incurs unacceptable CPU usage, right?
But if 180s is too long and 5s is too short then what does a “good”
number even look like? E.g.,. if 60s is deemed OK, then is there any
harm in just defining DEFAULT_NAPTIME_PER_CYCLE to be 60s and leaving
it at that?

Idea 2)

Another idea could be to use a “dynamic timeout” in the WaitLatch of
ApplyLauncherMain. Let me try to explain my thought bubble:
- If the preceding foreach(lc, sublist) loop launched any workers then
the WaitLatch timeout can be a much shorter 10s
- If the preceding foreach(lc, sublist) loop did NOT launch any
workers (this would be the most common case) then the WaitLatch
timeout can be the usual 180s.

IIUC this strategy will still give any recently launched workers
enough time to attach shmem but it also means that any concurrent
CREATE SUBSCRIPTION will be addressed within 10s instead of 180s.
Maybe this is sufficient to make an already rare problem become
insignificant.


> c) Don't reset the latch at worker attach and allow launcher main to
> identify and handle it. For this there is a patch v6-0002 available at
> [2].

This option c seems the easiest. Can you explain what are the
drawbacks of using this approach?

==
[1] github -  
https://github.com/postgres/postgres/commit/665d1fad99e7b11678b0d5fa24d2898424243cd6#diff-127f8eb009151ec548d14c877a57a89d67da59e35ea09189411aed529c6341bf

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Need clarification on compilation errors in PG 16.2

[Pradeep Kumar]

Hello Tom,

>That's correct for recent versions of macOS.  I see you are
>building against a recent SDK:
>
>/Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/string.h
>
>but I wonder if maybe the actual OS version is back-rev?

Currently Im using "MacOSX14.4.sdk" , path is
"/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/string.h".
When I go through the header file and search for the memset_s(), I found
that library is defined in a conditional macro refer below, am I breaking
the macro below?

#if defined(__STDC_WANT_LIB_EXT1__) && __STDC_WANT_LIB_EXT1__ >= 1
#include 
#include 

__BEGIN_DECLS
errno_t memset_s(void *__s, rsize_t __smax, int __c, rsize_t __n)
__OSX_AVAILABLE_STARTING(__MAC_10_9, __IPHONE_7_0);
__END_DECLS
#endif

Thanks and Regards
Pradeep

On Wed, May 29, 2024 at 2:21 AM Tom Lane  wrote:

> Pradeep Kumar  writes:
> > Yes it was defined in "pg_config.h"
> > /* Define to 1 if you have the `memset_s' function. */
> > #define HAVE_MEMSET_S 1
>
> That's correct for recent versions of macOS.  I see you are
> building against a recent SDK:
>
>
> /Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/string.h
>
> but I wonder if maybe the actual OS version is back-rev?
>
> regards, tom lane
>

Re: Need clarification on compilation errors in PG 16.2

[Pradeep Kumar]

Hi,

I found out that for using memset() library is not referred from
"/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/string.h"
, it referred from
"/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/secure/_string.h",
in that file didn't defined the memset_s() macro.

Thanks and regards
Pradeep

On Wed, May 29, 2024 at 11:30 AM Pradeep Kumar 
wrote:

> Hello Tom,
>
> >That's correct for recent versions of macOS.  I see you are
> >building against a recent SDK:
> >
>
> >/Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/string.h
> >
> >but I wonder if maybe the actual OS version is back-rev?
>
> Currently Im using "MacOSX14.4.sdk" , path is
> "/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/string.h".
> When I go through the header file and search for the memset_s(), I found
> that library is defined in a conditional macro refer below, am I breaking
> the macro below?
>
> #if defined(__STDC_WANT_LIB_EXT1__) && __STDC_WANT_LIB_EXT1__ >= 1
> #include 
> #include 
>
> __BEGIN_DECLS
> errno_t memset_s(void *__s, rsize_t __smax, int __c, rsize_t __n)
> __OSX_AVAILABLE_STARTING(__MAC_10_9, __IPHONE_7_0);
> __END_DECLS
> #endif
>
> Thanks and Regards
> Pradeep
>
> On Wed, May 29, 2024 at 2:21 AM Tom Lane  wrote:
>
>> Pradeep Kumar  writes:
>> > Yes it was defined in "pg_config.h"
>> > /* Define to 1 if you have the `memset_s' function. */
>> > #define HAVE_MEMSET_S 1
>>
>> That's correct for recent versions of macOS.  I see you are
>> building against a recent SDK:
>>
>>
>> /Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/include/string.h
>>
>> but I wonder if maybe the actual OS version is back-rev?
>>
>> regards, tom lane
>>
>

Re: meson: Specify -Wformat as a common warning flag for extensions

[Peter Eisentraut]

On 07.04.24 18:01, Sutou Kouhei wrote:

+# We don't have "warning_level == 3" and "warning_level ==
+# 'everything'" here because we don't use these warning levels.
+if warning_level == '1'
+  common_builtin_flags += ['-Wall']
+elif warning_level == '2'
+  common_builtin_flags += ['-Wall', '-Wextra']
+endif


I would trim this even further and always export just '-Wall'.  The 
other options aren't really something we support.


The other stanzas, on '-g' and '-O*', look good to me.

meson "experimental"?

[Peter Eisentraut]

In installation.sgml it says

"""
Alternatively, PostgreSQL can be built using Meson.  This is currently 
experimental.

"""

Do we want to alter this statement for PG17, considering that this is 
now the only way to build for Windows using MSVC?


(A joke response is that the Windows port itself is experimental, so it 
doesn't matter much that the build system for it is as well.)


I would still call Meson use on non-Windows platforms experimental at 
this time.


So maybe something like

"Alternatively, PostgreSQL can be built using Meson.  This is the only 
option for building PostgreSQL in Windows using Visual Something[*]. 
For other platforms, using Meson is currently experimental."


[*] What is the correct name for this?

Re: meson: Specify -Wformat as a common warning flag for extensions

[Sutou Kouhei]

Hi,

In <4707d4ed-f268-43c0-b4dd-cdbc7520f...@eisentraut.org>
  "Re: meson: Specify -Wformat as a common warning flag for extensions" on Tue, 
28 May 2024 23:31:05 -0700,
  Peter Eisentraut  wrote:

> On 07.04.24 18:01, Sutou Kouhei wrote:
>> +# We don't have "warning_level == 3" and "warning_level ==
>> +# 'everything'" here because we don't use these warning levels.
>> +if warning_level == '1'
>> +  common_builtin_flags += ['-Wall']
>> +elif warning_level == '2'
>> +  common_builtin_flags += ['-Wall', '-Wextra']
>> +endif
> 
> I would trim this even further and always export just '-Wall'.  The
> other options aren't really something we support.

OK. How about the v6 patch? It always uses '-Wall'.

Thanks,
-- 
kou
>From 8238adba3f3fc96d4a9e50af611b1cb3566abc0e Mon Sep 17 00:00:00 2001
From: Sutou Kouhei 
Date: Fri, 15 Mar 2024 18:27:30 +0900
Subject: [PATCH v6] meson: Restore implicit warning/debug/optimize flags for
 extensions

Meson specifies warning/debug/optimize flags such as "-Wall", "-g" and
"-O2" automatically based on "--warnlevel" and "--buildtype"
options. And we use "--warning_level=1" and
"--buildtype=debugoptimized" by default.

We don't specify warning/debug/optimize flags explicitly to build
PostgreSQL with Meson. Because Meson does it automatically as we said.
But Meson doesn't care about flags in Makefile.global and
pg_config. So we need to care about them manually.

This changes do it. They detect debug/optimize flags based on
debug/optimization option values because Meson doesn't tell us flags
Meson guessed. We always use -Wall for warning flags.
---
 meson.build | 27 +++
 src/include/meson.build |  4 ++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index d6401fb8e30..d7239dbb114 100644
--- a/meson.build
+++ b/meson.build
@@ -1851,6 +1851,33 @@ endif
 vectorize_cflags = cc.get_supported_arguments(['-ftree-vectorize'])
 unroll_loops_cflags = cc.get_supported_arguments(['-funroll-loops'])
 
+# They aren't used for building PostgreSQL itself because Meson does
+# everything internally. They are used by extensions via pg_config or
+# Makefile.global.
+common_builtin_flags = ['-Wall']
+
+if get_option('debug')
+  common_builtin_flags += ['-g']
+endif
+
+optimization = get_option('optimization')
+if optimization == '0'
+  common_builtin_flags += ['-O0']
+elif optimization == '1'
+  common_builtin_flags += ['-O1']
+elif optimization == '2'
+  common_builtin_flags += ['-O2']
+elif optimization == '3'
+  common_builtin_flags += ['-O3']
+elif optimization == 's'
+  common_builtin_flags += ['-Os']
+endif
+
+cflags_builtin = cc.get_supported_arguments(common_builtin_flags)
+if llvm.found()
+  cxxflags_builtin = cpp.get_supported_arguments(common_builtin_flags)
+endif
+
 common_warning_flags = [
   '-Wmissing-prototypes',
   '-Wpointer-arith',
diff --git a/src/include/meson.build b/src/include/meson.build
index a28f115d867..58b7a9c1e7e 100644
--- a/src/include/meson.build
+++ b/src/include/meson.build
@@ -44,9 +44,9 @@ config_paths_data.set_quoted('MANDIR', dir_prefix / dir_man)
 
 var_cc = ' '.join(cc.cmd_array())
 var_cpp = ' '.join(cc.cmd_array() + ['-E'])
-var_cflags = ' '.join(cflags + cflags_warn + get_option('c_args'))
+var_cflags = ' '.join(cflags + cflags_builtin + cflags_warn + get_option('c_args'))
 if llvm.found()
-  var_cxxflags = ' '.join(cxxflags + cxxflags_warn + get_option('cpp_args'))
+  var_cxxflags = ' '.join(cxxflags + cxxflags_builtin + cxxflags_warn + get_option('cpp_args'))
 else
   var_cxxflags = ''
 endif
-- 
2.43.0