Re: RFC: pg_stat_logmsg

[Gurjeet Singh]

On Thu, Jul 6, 2023 at 12:37 AM Masahiko Sawada  wrote:
> On Sat, Jul 1, 2023 at 8:57 AM Joe Conway  wrote:
> >
> > The basic idea is to mirror how pg_stat_statements works, except the
> > logged messages keyed by filename, lineno, and elevel are saved with a
> > aggregate count. The format string is displayed (similar to a query
> > jumble) for context, along with function name and sqlerrcode.
> >
> >
> > Part of the thinking is that people with fleets of postgres instances
> > can use this to scan for various errors that they care about.
> > Additionally it would be useful to look for "should not happen" errors.

> I'm concerned that displaying the format string could not be helpful
> in some cases. For example, when raising an ERROR while reading WAL
> records, we typically write the error message stored in
> record->errormsg_buf:
>
> in ReadRecord():
> if (errormsg)
> ereport(emode_for_corrupt_record(emode, 
> xlogreader->EndRecPtr),
> (errmsg_internal("%s", errormsg) /* already
> translated */ ));
>
> In this case, the error message stored in pg_stat_logmsg is just '%s'.
> The filename and line number columns might help identify the actual
> error but it requires users to read the source code and cannot know
> the actual error message.

I believe that the number of such error messages, the ones with very
little descriptive content, is very low in Postgres code. These kinds
of messages are not prevalent, and must be used when code hits obscure
conditions, like seen in your example above. These are the kinds of
errors that Joe is referring to as "should not happen". In these
cases, even if the error message was descriptive, the user will very
likely have to dive deep into code to find out the real cause.

I feel that the unique combination of file name and line number is
useful information, even in cases where the format string not very
descriptive. So I believe the extension's behaviour in this regard is
acceptable.

In cases where the extension's output is not descriptive enough, the
user can use the filename:lineno pair to look for fully formed error
messages in the actual log files; they may have to make appropriate
changes to log_* parameters, though.

If we still feel strongly about getting the actual message for these
cases, perhaps we can develop a heuristic to identify such messages,
and use either full or a prefix of the 'message' field, instead of
'message_id' field. The heuristic could be: strlen(message_id) is too
short, or that message_id is all/overwhelmingly format specifiers
(e.g. '%s: %d').

The core benefit of this extension is to make it easy for the user to
discover error messages affecting their workload. The user may be
interested in knowing the most common log messages in their server
logs, so that they can work on reducing those errors or warnings. Or
they may be interested in knowing when their workload hits
unexpected/serious error messages, even if they're infrequent. The
data exposed by pg_stat_logmsg view would serve as a starting point,
but I'm guessing it would not be sufficient for their investigation.

On Fri, Jun 30, 2023 at 4:57 PM Joe Conway  wrote:
> I would love to get
> feedback on:
>
> 1/ the general concept
> 2/ the pg_stat_statement-like implementation
> 3/ contrib vs core vs external project

+1 for the idea; a monitoring system trained at this view can bubble
up problems to users that may otherwise go unnoticed. Piggybacking on,
or using pg_stat_statments implementation as a model seems fine. I
believe making this available as a contrib module hits the right
balance; not all installations may want this, hence in-core, always
collecting data, seems undesirable; if it's an external project, it
won't be discoverable, I see that as a very high bar for someone to
trust it and begin to use it.

Best regards,
Gurjeet
http://Gurje.et

Re: pgsql: Fix search_path to a safe value during maintenance operations.

[Nathan Bossart]

Here is a new version of the patch that I think is ready for commit (except
it still needs a catversion bump).  The only real difference from v1 is in
AdjustUpgrade.pm.  From my cross-version pg_upgrade testing, I believe we
can remove the other "privilege-set discrepancy" rule as well.

Since MAINTAIN will no longer exist in v16, we'll also need the following
change applied to v17devel:

diff --git a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm 
b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
index 843f65b448..d435812c06 100644
--- a/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ b/src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -274,7 +274,7 @@ sub adjust_old_dumpfile
$dump = _mash_view_qualifiers($dump);
}
 
-   if ($old_version >= 14 && $old_version < 16)
+   if ($old_version >= 14 && $old_version < 17)
{
# Fix up some privilege-set discrepancies.
$dump =~

On Thu, Jul 06, 2023 at 10:20:04AM -0700, Nathan Bossart wrote:
> On Thu, Jul 06, 2023 at 12:55:14AM -0700, Jeff Davis wrote:
>> Also remember to bump the catversion. Other than that, it looks good to
>> me.
> 
> Will do.

Since we are only reverting from v16, the REL_16_STABLE catversion will be
bumped ahead of the one on master.  AFAICT that is okay, but there is also
a chance that someone bumps the catversion on master to the same value.
I'm not sure if this is problem is worth worrying about, but I thought I'd
raise it just in case.  I could bump the catversion on master to the
following value to help prevent this scenario, but I'm not wild about
adding unnecessary catversion bumps.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From c9e6c96fd7f9576470042efb196ebd12eaf66442 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Thu, 6 Jul 2023 21:35:33 -0700
Subject: [PATCH v2 1/1] Revert MAINTAIN privilege and pg_maintain predefined
 role.

This reverts the following commits: 4dbdb82513, c2122aae63,
5b1a879943, 9e1e9d6560, ff9618e82a, 60684dd834, 4441fc704d,
and b5d6382496.  A role with the MAINTAIN privilege may be able to
use search_path tricks to escalate privileges to the table owner.
Unfortunately, it is too late in the v16 development cycle to fix
this by restricting search_path when running maintenance commands.
As of this writing, the plan is to revert this feature from only
v16 and to apply a fix to v17devel.

Bumps catversion.

Reviewed-by: Jeff Davis
Discussion: https://postgr.es/m/E1q7j7Y-000z1H-Hr%40gemulon.postgresql.org
---
 doc/src/sgml/ddl.sgml |  35 ++
 doc/src/sgml/func.sgml|   2 +-
 .../sgml/ref/alter_default_privileges.sgml|   4 +-
 doc/src/sgml/ref/analyze.sgml |   6 +-
 doc/src/sgml/ref/cluster.sgml |  10 +-
 doc/src/sgml/ref/grant.sgml   |   3 +-
 doc/src/sgml/ref/lock.sgml|   4 +-
 .../sgml/ref/refresh_materialized_view.sgml   |   5 +-
 doc/src/sgml/ref/reindex.sgml |  23 ++--
 doc/src/sgml/ref/revoke.sgml  |   2 +-
 doc/src/sgml/ref/vacuum.sgml  |   6 +-
 doc/src/sgml/user-manag.sgml  |  12 --
 src/backend/catalog/aclchk.c  |  15 ---
 src/backend/commands/analyze.c|  13 +-
 src/backend/commands/cluster.c|  43 ++-
 src/backend/commands/indexcmds.c  |  34 +++--
 src/backend/commands/lockcmds.c   |   2 +-
 src/backend/commands/matview.c|   3 +-
 src/backend/commands/tablecmds.c  |  16 ++-
 src/backend/commands/vacuum.c |  65 +-
 src/backend/utils/adt/acl.c   |   8 --
 src/bin/pg_dump/dumputils.c   |   1 -
 src/bin/pg_dump/t/002_pg_dump.pl  |   2 +-
 src/bin/psql/tab-complete.c   |   6 +-
 src/include/catalog/pg_authid.dat |   5 -
 src/include/commands/tablecmds.h  |   5 +-
 src/include/commands/vacuum.h |   5 +-
 src/include/nodes/parsenodes.h|   3 +-
 src/include/utils/acl.h   |   5 +-
 .../expected/cluster-conflict-partition.out   |   8 +-
 .../specs/cluster-conflict-partition.spec |   2 +-
 .../perl/PostgreSQL/Test/AdjustUpgrade.pm |  11 --
 src/test/regress/expected/cluster.out |   7 --
 src/test/regress/expected/create_index.out|   4 +-
 src/test/regress/expected/dependency.out  |  22 ++--
 src/test/regress/expected/privileges.out  | 116 --
 src/test/regress/expected/rowsecurity.out |  34 ++---
 src/test/regress/sql/cluster.sql  |   5 -
 src/test/regress/sql/dependency.sql   |   2 +-
 src/test/regress/sql/privileges.sql   |  68 --
 40 files changed, 178 insertions(+), 444 deletions(-)

diff 

Add hint message for check_log_destination()

[Japin Li]

Hi, hackers

When I try to change log_destination using ALTER SYSTEM with the wrong value,
it complains of the "Unrecognized key word" without available values.  This
patch tries to add a hint message that provides available values for
log_destination.  Any thoughts?

-- 
Regrads,
Japin Li.

>From 7d6b50c9deaba576cdc28e170bff541f630415f9 Mon Sep 17 00:00:00 2001
From: Japin Li 
Date: Fri, 7 Jul 2023 12:59:09 +0800
Subject: [PATCH v1 1/1] Add hint message for log_destination

---
 src/backend/utils/error/elog.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..fdc6557a59 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2228,8 +2228,21 @@ check_log_destination(char **newval, void **extra, GucSource source)
 #endif
 		else
 		{
+			StringInfoData errhint;
+
+			initStringInfo();
+			appendStringInfo(, "stderr, csvlog, jsonlog");
+#ifdef HAVE_SYSLOG
+			appendStringInfo(, ", syslog");
+#endif
+#ifdef WIN32
+			appendStringInfo(, ", eventlog");
+#endif
+
 			GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+			GUC_check_errhint("Available values: %s.", errhint.data);
 			pfree(rawstring);
+			pfree(errhint.data);
 			list_free(elemlist);
 			return false;
 		}
-- 
2.25.1

Re: Disabling Heap-Only Tuples

[Dilip Kumar]

On Fri, Jul 7, 2023 at 1:48 AM Matthias van de Meent
 wrote:
>
> On Wed, 5 Jul 2023 at 19:55, Thom Brown  wrote:
> >
> > On Wed, 5 Jul 2023 at 18:05, Matthias van de Meent
> >  wrote:
> > > So what were you thinking of? A session GUC? A table option?
> >
> > Both.
>
> Here's a small patch implementing a new table option max_local_update
> (name very much bikesheddable). Value is -1 (default, disabled) or the
> size of the table in MiB that you still want to allow to update on the
> same page. I didn't yet go for a GUC as I think that has too little
> control on the impact on the system.

So IIUC, this parameter we can control that instead of putting the new
version of the tuple on the same page, it should choose using
RelationGetBufferForTuple(), and that can reduce the fragmentation
because now if there is space then most of the updated tuple will be
inserted in same pages.  But this still can not truncate the pages
from the heap right? because we can not guarantee that the new page
selected by RelationGetBufferForTuple() is not from the end of the
heap, and until we free the pages from the end of the heap, the vacuum
can not truncate any page.  Is my understanding correct?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Autogenerate some wait events code and documentation

[Michael Paquier]

On Thu, Jul 06, 2023 at 06:19:43PM -0700, Andres Freund wrote:
> On 2023-07-06 09:36:12 +0900, Michael Paquier wrote:
>> So you mean renaming the existing events like WalSenderWaitForWAL to
>> WalSenderWaitForWal?
> 
> Yes.
>
>> The impact on existing monitoring queries is not zero because any changes
>> would be silent, and that's the part that worried me the most even if it can
>> remove one column in the txt file.
> 
> Then let's just use - or so to indicate the inferred name, with a "string"
> overriding it?

Hmm.  If we go down this road I would make the choice of simplicity
and remove entirely a column, then, generating the snakecase from the
camelcase or vice-versa (say like a $string =~ s/([a-z]+)/$1_/g;),
even if it means having slightly incompatible strings showing to the
users. And I'd rather minimize the number of exceptions we need to
handle in this automation (aka no exception rules for some keywords
like "SSL" or "WAL", etc.).
--
Michael


signature.asc
Description: PGP signature

Re: Fix search_path for all maintenance commands

[Isaac Morland]

On Thu, 6 Jul 2023 at 21:39, Jeff Davis  wrote:

I apologize in advance if anything I’ve written below is either too obvious
or too crazy or misinformed to belong here. I hope I have something to say
that is on point, but feel unsure what makes sense to say.

* It might break for users who have a functional index where the
> function implicitly depends on a search_path containing a namespace
> other than pg_catalog. My opinion is that such functional indexes are
> conceptually broken and we need to desupport them, and there will be
> some breakage, but I'm open to suggestion about how we minimize that (a
> compatibility GUC or something?).
>

I agree this is OK. If somebody has an index whole meaning depends on the
search_path, then the best that can be said is that their database hasn't
been corrupted yet. At the same time, I can see that somebody would get
upset if they couldn't upgrade their database because of this. Maybe
pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp" to any
function used in a functional index that doesn't have a search_path setting
of its own? (BEGIN ATOMIC functions count, if I understand correctly, as
having a search_path setting, because the lookups happen at definition time)

Now I'm doing more reading and I'm worried about SET TIME ZONE (or more
precisely, its absence) and maybe some other ones.

* The fix might not go far enough or might be in the wrong place. I'm
> open to suggestion here, too. Maybe we can make it part of the general
> function call mechanism, and can be overridden by explicitly setting
> the function search path? Or maybe we need new syntax where the
> function can acquire the search path from the session explicitly, but
> uses a safe search path by default?
>

Change it so by default each function gets handled as if "SET search_path
FROM CURRENT" was applied to it? That's what I do for all my functions
(maybe hurting performance?). Expand on my pg_upgrade idea above by
applying it to all functions?

I feel that this may tie into other behaviour issues where to me it is
obvious that the expected behaviour should be different from the actual
behaviour. If a view calls a function, shouldn't it be called in the
context of the view's definer/owner? It's weird that I can write a view
that filters a table for users of the view, but as soon as the view calls
functions they run in the security context of the user of the view. Are
views security definers or not? Similar comment for triggers. Also as far
as I can tell there is no way for a security definer function to determine
who (which user) invoked it. So I can grant/deny access to run a particular
function using permissions, but I can't have the supposed security definer
define security for different callers.

Is the fundamental problem that we now find ourselves wanting to do things
that require different defaults to work smoothly? On some level I suspect
we want lexical scoping, which is what most of us have in our programming
languages, in the database; but the database has many elements of dynamic
scoping, and changing that is both a compatibility break and requires
significant changes in the way the database is designed.

Re: POC, WIP: OR-clause support for indexes

[Andrey Lepikhov]

On 6/7/2023 03:06, Alena Rybakina wrote:
The test was performed on the same benchmark database generated by 2 
billion values.


I corrected this constant in the patch.
In attempt to resolve some issues had mentioned in my previous letter I 
used op_mergejoinable to detect mergejoinability of a clause.
Constant side of the expression is detected by call of 
eval_const_expressions() and check each side on the Const type of node.


See 'diff to diff' in attachment.

--
regards,
Andrey Lepikhov
Postgres Professional
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 26648b0876..67d6f85010 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -111,38 +111,27 @@ transformBoolExprOr(ParseState *pstate, BoolExpr 
*expr_orig)
 {
List   *or_list = NIL;
List   *groups_list = NIL;
-   ListCell   *lc_eargs;
+   ListCell   *lc;
Node   *result;
BoolExpr   *expr;
boolchange_apply = false;
 
-   /* If this is not expression "Or", then will do it the old way. */
+   /* If this is not an 'OR' expression, skip the transformation */
if (expr_orig->boolop != OR_EXPR ||
list_length(expr_orig->args) < const_transform_or_limit)
-   return transformBoolExpr(pstate, (BoolExpr *)expr_orig);
+   return transformBoolExpr(pstate, (BoolExpr *) expr_orig);
 
expr = (BoolExpr *)copyObject(expr_orig);
 
-   /*
-   * NOTE:
-   * It is an OR-clause. So, rinfo->orclause is a BoolExpr node, 
contains
-   * a list of sub-restrictinfo args, and rinfo->clause - which is 
the
-   * same expression, made from bare clauses. To not break 
selectivity
-   * caches and other optimizations, use both:
-   * - use rinfos from orclause if no transformation needed
-   * - use  bare quals from rinfo->clause in the case of 
transformation,
-   * to create new RestrictInfo: in this case we have no options 
to avoid
-   * selectivity estimation procedure.
-   */
-   foreach(lc_eargs, expr->args)
+   foreach(lc, expr->args)
{
-   A_Expr *arg = (A_Expr *) lfirst(lc_eargs);
-   Node   *bare_orarg;
+   Node   *arg = lfirst(lc);
+   Node   *orqual;
Node   *const_expr;
-   Node   *non_const_expr;
+   Node   *nconst_expr;
ListCell   *lc_groups;
OrClauseGroupEntry *gentry;
-   boolallow_transformation;
+   boolconst_is_left = true;
 
/*
 * The first step: checking that the expression consists only 
of equality.
@@ -154,33 +143,51 @@ transformBoolExprOr(ParseState *pstate, BoolExpr 
*expr_orig)
 * same level of a single BoolExpr expression, otherwise all of 
them cannot be converted.
 */
 
-   if (!arg)
-   break;
+   orqual = transformExprRecurse(pstate, (Node *) arg);
+   orqual = coerce_to_boolean(pstate, orqual, "OR");
+   orqual = eval_const_expressions(NULL, orqual);
 
-   allow_transformation = (arg->type == T_A_Expr && (arg)->kind == 
AEXPR_OP &&
-   
list_length((arg)->name) >=1 && strcmp(strVal(linitial((arg)->name)), "=") == 0
-  );
+   if (!IsA(orqual, OpExpr))
+   {
+   or_list = lappend(or_list, orqual);
+   continue;
+   }
 
+   /* Detect constant side of the clause */
+   if (IsA(get_leftop(orqual), Const))
+   const_is_left = true;
+   else if (IsA(get_rightop(orqual), Const))
+   const_is_left = false;
+   else
+   {
+   or_list = lappend(or_list, orqual);
+   continue;
+   }
 
-   bare_orarg = transformExprRecurse(pstate, (Node *)arg);
-   bare_orarg = coerce_to_boolean(pstate, bare_orarg, "OR");
+   /* Get pointers to constant and expression sides of the qual */
+   nconst_expr = (const_is_left) ? get_rightop(orqual) :
+   
get_leftop(orqual);
+   const_expr = (const_is_left) ?  get_leftop(orqual) :
+   
get_rightop(orqual);
+

Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

[Peter Smith]

Hi, Here are my review comments for patch v2.

==
1. doc/src/sgml/logical-replication.sgml

Candidate indexes must be btree, non-partial, and have at least one
column reference to the published table column at the leftmost column
(i.e. cannot consist of only expressions).

~

There is only one column which can be the leftmost one, so the wording
"At least one ... at the leftmost"  seemed a bit strange to me.
Personally, I would phrase it something like below:

SUGGESTION #1
Candidate indexes must be btree, non-partial, and the leftmost index
column must reference a published table column (i.e. the index cannot
consist of only expressions).

SUGGESTION #2 (same as above, but omitting the "only expressions"
part, which I think is implied by the "leftmost" rule anyway)
Candidate indexes must be btree, non-partial, and the leftmost index
column must reference a published table column.

==
2. src/backend/replication/logical/relation.c

  * Returns the oid of an index that can be used by the apply worker to scan
  * the relation. The index must be btree, non-partial, and have at least
- * one column reference (i.e. cannot consist of only expressions). These
- * limitations help to keep the index scan similar to PK/RI index scans.
+ * one column reference to the remote relation's column at the leftmost column
+ * (i.e. cannot consist of only expressions). These limitations help
to keep the
+ * index scan similar to PK/RI index scans.

This comment text is similar to the docs change, so refer to the same
suggestions as #1 above.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Fix search_path for all maintenance commands

[Jeff Davis]

On Fri, 2023-05-26 at 16:21 -0700, Jeff Davis wrote:
> Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW,
> REINDEX, and VACUUM) currently run as the table owner, and as a
> SECURITY_RESTRICTED_OPERATION.
> 
> I propose that we also fix the search_path to "pg_catalog, pg_temp"
> when running maintenance commands

New patch attached.

We need this patch for several reasons:

* If you have a functional index, and the function depends on the
search_path, then it's easy to corrupt your index if you (or a
superuser) run a REINDE/CLUSTER with the wrong search_path.

* The MAINTAIN privilege needs a safe search_path, and was reverted
from 16 because the search_path in 16 is not restricted.

* In general, it's a good idea for things like functional indexes and
materialized views to be independent of the search_path.

* The search_path is already restricted in some other contexts, like
logical replication and autoanalyze.

Others have raised some concerns though:

* It might break for users who have a functional index where the
function implicitly depends on a search_path containing a namespace
other than pg_catalog. My opinion is that such functional indexes are
conceptually broken and we need to desupport them, and there will be
some breakage, but I'm open to suggestion about how we minimize that (a
compatibility GUC or something?).

* The fix might not go far enough or might be in the wrong place. I'm
open to suggestion here, too. Maybe we can make it part of the general
function call mechanism, and can be overridden by explicitly setting
the function search path? Or maybe we need new syntax where the
function can acquire the search path from the session explicitly, but
uses a safe search path by default?


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS


From 46dbbf86e927e1872004c37f651e6a5b5c62bdd0 Mon Sep 17 00:00:00 2001
From: Jeff Davis 
Date: Thu, 6 Jul 2023 13:06:22 -0700
Subject: [PATCH v5] Restrict search_path when performing maintenance.

When executing maintenance operations (ANALYZE, CLUSTER, REFRESH
MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to
'pg_catalog, pg_temp'.

Functions that are used for functional indexes, in index expressions,
or in materialized views and depend on a different search path should
be declared with CREATE FUNCTION ... SET search_path='...'.

This change was previously committed as 05e1737351, but was not
acceptable for 16 and reverted.

Discussion: https://postgr.es/m/CA%2BTgmoZVCHERUkXhAMT2Er-sKBc5C6_iX%2BTpxxivBevDHzq2TQ%40mail.gmail.com
Discussion: https://postgr.es/m/e44327179e5c9015c8dda67351c04da552066017.camel%40j-davis.com
---
 contrib/amcheck/verify_nbtree.c  |  2 ++
 src/backend/access/brin/brin.c   |  2 ++
 src/backend/catalog/index.c  |  8 
 src/backend/commands/analyze.c   |  2 ++
 src/backend/commands/cluster.c   |  2 ++
 src/backend/commands/indexcmds.c |  6 ++
 src/backend/commands/matview.c   |  2 ++
 src/backend/commands/vacuum.c|  2 ++
 src/bin/scripts/t/100_vacuumdb.pl|  4 
 src/include/utils/guc.h  |  6 ++
 .../test_oat_hooks/expected/test_oat_hooks.out   |  4 
 src/test/regress/expected/privileges.out | 12 ++--
 src/test/regress/expected/vacuum.out |  2 +-
 src/test/regress/sql/privileges.sql  |  8 
 src/test/regress/sql/vacuum.sql  |  2 +-
 15 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 94a9759322..35035967f9 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -281,6 +281,8 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
 		SetUserIdAndSecContext(heaprel->rd_rel->relowner,
 			   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 		save_nestlevel = NewGUCNestLevel();
+		SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+		PGC_S_SESSION);
 	}
 	else
 	{
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 3c6a956eaa..11e78cb62c 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1066,6 +1066,8 @@ brin_summarize_range(PG_FUNCTION_ARGS)
 		SetUserIdAndSecContext(heapRel->rd_rel->relowner,
 			   save_sec_context | SECURITY_RESTRICTED_OPERATION);
 		save_nestlevel = NewGUCNestLevel();
+		SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET,
+		PGC_S_SESSION);
 	}
 	else
 	{
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 67b743e251..64127a894f 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1476,6 +1476,8 @@ index_concurrently_build(Oid heapRelationId,
 	

Re: Autogenerate some wait events code and documentation

[Andres Freund]

Hi,

On 2023-07-06 09:36:12 +0900, Michael Paquier wrote:
> On Wed, Jul 05, 2023 at 02:59:39PM -0700, Andres Freund wrote:
> > Rebasing a patch over this I was a bit confused because I got a bunch of
> > ""unable to parse wait_event_names.txt" errors. Took me a while to figure 
> > out
> > that that was just because I didn't include a trailing . in the description.
> > Perhaps that could be turned into a more meaningful error?
> > 
> > die "unable to parse wait_event_names.txt"
> >   unless $line =~ /^(\w+)\t+(\w+)\t+("\w+")\t+("\w.*\.")$/;
> 
> Agreed that we could at least add the $line in the error message
> generated, at least, to help with debugging.
> 
> > I also do wonder if we should invest in generating the lwlock names as
> > well. Except for a few abbreviations, the second column is always the
> > camel-cased version of what follows WAIT_EVENT_. Feels pretty tedious to 
> > write
> > that out.
> 
> And you mean getting rid of lwlocknames.txt?

No, I meant the second column in wait_event_names.txt. If you look at stuff
like:
WAIT_EVENT_ARCHIVER_MAIN"ArchiverMain"  "Waiting in main loop of 
archiver process."

It'd be pretty trivial to generate ArchiverMain from ARCHIVER_MAIN.



> The impact on dtrace or other similar tools is uncertain to me because we
> have free number on this list, and stuff like GetLWLockIdentifier() rely on
> the input ID from lwlocknames.txt.

I don't really care, tbh. If we wanted to keep the names the same in case of
abbreviations, we could just make the name optional, and auto-generated if not
explicitly specified.



> > Perhaps we should just change the case of the upper-cased names (DSM, SSL,
> > WAL, ...) to follow the other names?
> 
> So you mean renaming the existing events like WalSenderWaitForWAL to
> WalSenderWaitForWal?

Yes.


> The impact on existing monitoring queries is not zero because any changes
> would be silent, and that's the part that worried me the most even if it can
> remove one column in the txt file.

Then let's just use - or so to indicate the inferred name, with a "string"
overriding it?

Greetings,

Andres Freund

Re: HOT readme missing documentation on summarizing index handling

[Tomas Vondra]

Yeah, README.HOT should have been updated, and I see no reason not to
backpatch this to v16. Barring objections, I'll do that tomorrow.

I have two suggesting regarding the README.HOT changes:

1) I'm not entirely sure it's very clear what "referential integrity of
indexes across tuple updates" actually means. I'm afraid "referential
integrity" may lead readers to think about foreign keys. Maybe it'd be
better to explain this is about having index pointers to the new tuple
version, etc.

2) Wouldn't it be good to make it a bit more explicit we now have three
"levels" of HOT:

  (a) no indexes need update
  (b) update only summarizing indexes
  (c) update all indexes

The original text was really about on/off, and I'm not quite sure the
part about "exception" makes this very clear.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [PoC] Federated Authn/z with OAUTHBEARER

[Thomas Munro]

On Fri, Jul 7, 2023 at 4:57 AM Jacob Champion  wrote:
> On Wed, Jul 5, 2023 at 3:07 PM Thomas Munro  wrote:
> > 2.  Convert those events into new libpq events like 'I want you to
> > call me back in 100ms', and 'call me back when socket #42 has data',
> > and let clients handle that by managing their own poll set etc.  (This
> > is something I've speculated about to support more efficient
> > postgres_fdw shard query multiplexing; gotta figure out how to get
> > multiple connections' events into one WaitEventSet...)
>
> Something analogous to libcurl's socket and timeout callbacks [1],
> then? Or is there an existing libpq API you were thinking about using?

Yeah.  Libpq already has an event concept.  I did some work on getting
long-lived WaitEventSet objects to be used in various places, some of
which got committed[1], but not yet the parts related to postgres_fdw
(which uses libpq connections to talk to other PostgreSQL servers, and
runs into the limitations of PQsocket()).  Horiguchi-san had the good
idea of extending the event system to cover socket changes, but I
haven't actually tried it yet.  One day.

> > Or, more likely in the
> > first version, you just can't do it at all...  Doesn't seem that bad
> > to me.
>
> Any initial opinions on whether it's worse or better than a worker thread?

My vote is that it's perfectly fine to make a new feature that only
works on some OSes.  If/when someone wants to work on getting it going
on Windows/AIX/Solaris (that's the complete set of no-epoll, no-kqueue
OSes we target), they can write the patch.

[1] 
https://www.postgresql.org/message-id/flat/CA%2BhUKGJAC4Oqao%3DqforhNey20J8CiG2R%3DoBPqvfR0vOJrFysGw%40mail.gmail.com

Clean up some signal usage mainly related to Windows

[Tristan Partin]

Windows has support for some signals[0], like SIGTERM and SIGINT. SIGINT
must be handled with care on Windows since it is handled in a separate
thread. SIGTERM however can be handled in a similar way to UNIX-like
systems. I audited a few pqsignal calls that were blocked by WIN32 to
see if they could become used, and made some adjustments. Definitely
hoping for someone with more Windows knowledge to audit this.

In addition, I found that signal_cleanup() in pg_test_fsync.c was not
using signal-safe functions, so I went ahead and fixed those to use
their signal-safe equivalents.

[0]: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal

-- 
Tristan Partin
Neon (https://neon.tech)
From 2c4573afe55e83d3b5c99460c7429ff7cf3adcdf Mon Sep 17 00:00:00 2001
From: Tristan Partin 
Date: Thu, 6 Jul 2023 15:25:14 -0500
Subject: [PATCH v1 1/2] Use signal-safe functions in signal handler

According to signal-safety(7), exit(3) and puts(3) are not safe to call
in a signal handler.
---
 src/bin/pg_test_fsync/pg_test_fsync.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 435df8d808..2fc4b69444 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -596,8 +596,8 @@ signal_cleanup(SIGNAL_ARGS)
 	if (needs_unlink)
 		unlink(filename);
 	/* Finish incomplete line on stdout */
-	puts("");
-	exit(1);
+	write(STDOUT_FILENO, "", 1);
+	_exit(1);
 }
 
 #ifdef HAVE_FSYNC_WRITETHROUGH
-- 
Tristan Partin
Neon (https://neon.tech)

From ce4cbf8e720301e7ddbd2f0816c1f03c82138515 Mon Sep 17 00:00:00 2001
From: Tristan Partin 
Date: Thu, 6 Jul 2023 15:17:36 -0500
Subject: [PATCH v1 2/2] Cleanup some signal usage on Windows

SIGTERM can be handled the same as on a UNIX-like system. SIGINT, on the
other hand, requires special considerations. The Windows documentation
says the following:

> SIGINT is not supported for any Win32 application. When a CTRL+C
> interrupt occurs, Win32 operating systems generate a new thread to
> specifically handle that interrupt. This can cause a single-thread
> application, such as one in UNIX, to become multithreaded and cause
> unexpected behavior.

Therefore, if all the SIGINT handler does is read from memory and/or
exit, we can use the same handler. If the signal handler writes to
memory, such as setting a boolean, the original thread would never
become aware of the changed state.
---
 src/bin/initdb/initdb.c| 17 -
 src/bin/pg_basebackup/pg_receivewal.c  |  5 +
 src/bin/pg_basebackup/pg_recvlogical.c |  9 -
 src/bin/pg_test_fsync/pg_test_fsync.c  |  3 ---
 4 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f4167682a..cac58eae2a 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2773,26 +2773,17 @@ void
 setup_signals(void)
 {
 	/* some of these are not valid on Windows */
-#ifdef SIGHUP
-	pqsignal(SIGHUP, trapsig);
-#endif
-#ifdef SIGINT
+	pqsignal(SIGTERM, trapsig);
+
+#ifndef WIN32
 	pqsignal(SIGINT, trapsig);
-#endif
-#ifdef SIGQUIT
+	pqsignal(SIGHUP, trapsig);
 	pqsignal(SIGQUIT, trapsig);
-#endif
-#ifdef SIGTERM
-	pqsignal(SIGTERM, trapsig);
-#endif
 
 	/* Ignore SIGPIPE when writing to backend, so we can clean up */
-#ifdef SIGPIPE
 	pqsignal(SIGPIPE, SIG_IGN);
-#endif
 
 	/* Prevent SIGSYS so we can probe for kernel calls that might not work */
-#ifdef SIGSYS
 	pqsignal(SIGSYS, SIG_IGN);
 #endif
 }
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index d0a4079d50..55143c4037 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -611,14 +611,11 @@ StreamLog(void)
  * When SIGINT/SIGTERM are caught, just tell the system to exit at the next
  * possible moment.
  */
-#ifndef WIN32
-
 static void
 sigexit_handler(SIGNAL_ARGS)
 {
 	time_to_stop = true;
 }
-#endif
 
 int
 main(int argc, char **argv)
@@ -838,9 +835,9 @@ main(int argc, char **argv)
 	 * Trap signals.  (Don't do this until after the initial password prompt,
 	 * if one is needed, in GetConnection.)
 	 */
+	pqsignal(SIGTERM, sigexit_handler);
 #ifndef WIN32
 	pqsignal(SIGINT, sigexit_handler);
-	pqsignal(SIGTERM, sigexit_handler);
 #endif
 
 	/*
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f3c7937a1d..1228dc03db 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -644,10 +644,6 @@ error:
 	conn = NULL;
 }
 
-/*
- * Unfortunately we can't do sensible signal handling on windows...
- */
-#ifndef WIN32
 
 /*
  * When SIGINT/SIGTERM are caught, just tell the system to exit at the next
@@ -659,6 +655,9 @@ sigexit_handler(SIGNAL_ARGS)
 	time_to_abort = true;
 }
 
+/* SIGHUP is not defined on Windows */
+#ifndef WIN32
+
 /*
  * Trigger the output file to be reopened.
  

Re: Disabling Heap-Only Tuples

[Matthias van de Meent]

On Wed, 5 Jul 2023 at 19:55, Thom Brown  wrote:
>
> On Wed, 5 Jul 2023 at 18:05, Matthias van de Meent
>  wrote:
> > So what were you thinking of? A session GUC? A table option?
>
> Both.

Here's a small patch implementing a new table option max_local_update
(name very much bikesheddable). Value is -1 (default, disabled) or the
size of the table in MiB that you still want to allow to update on the
same page. I didn't yet go for a GUC as I think that has too little
control on the impact on the system.

I decided that max_local_update would be in MB because there is no
reloption value that can contain MaxBlockNumber and -1/disabled; and 1
MiB seems like enough granularity for essentially all use cases.

The added regression tests show how this feature works, that the new
feature works, and validate that lock levels are acceptable
(ShareUpdateExclusiveLock, same as for updating fillfactor).


Kind regards,

Matthias van de Meent
Neon (https://neon.tech/)


v1-0001-Implement-a-reloption-that-forces-updated-tuples-.patch
Description: Binary data

Re: warn if GUC set to an invalid shared library

[Justin Pryzby]

On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby  wrote:
> > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> > > > > It caused no issue when I changed:
> > > > >
> > > > > /* Check that it's acceptable for the 
> > > > > indicated parameter */
> > > > > if (!parse_and_validate_value(record, name, 
> > > > > value,
> > > > > - PGC_S_FILE, 
> > > > > ERROR,
> > > > > + PGC_S_TEST, 
> > > > > ERROR,
> > > > >   , 
> > > > > ))
> > > > >
> > > > > I'm not sure where to go from here.
> > > >
> > > > I'm hoping for some guidance ; this simple change may be naive, but I'm 
> > > > not
> > > > sure what a wider change would look like.

I'm still hoping.

> > PGC_S_TEST is a better fit, so my question is whether it's really that
> > simple ?  
> 
> I've added the trivial change as 0001 and re-opened the patch (which ended
> up in January's CF)
> 
> If for some reason it's not really as simple as that, then 001 will
> serve as a "straw-man patch" hoping to elicit discussion on that point.

> From defdb57fe0ec373c1eea8df42f0e1831b3f9c3cc Mon Sep 17 00:00:00 2001
> From: Justin Pryzby 
> Date: Fri, 22 Jul 2022 15:52:11 -0500
> Subject: [PATCH v6 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not
>  FILE
> 
> WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE
> 
> Since the value didn't come from a file.  Or maybe we should have
> another PGC_S_ value for this, or a flag for 'is a test'.
> ---
>  src/backend/utils/misc/guc.c | 2 +-
>  src/include/utils/guc.h  | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index 6f21752b844..ae8810591d6 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -4435,7 +4435,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
>  
>   /* Check that it's acceptable for the indicated 
> parameter */
>   if (!parse_and_validate_value(record, name, value,
> - 
>   PGC_S_FILE, ERROR,
> + 
>   PGC_S_TEST, ERROR,
>   
>   , ))
>   ereport(ERROR,
>   
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),

This is rebased over my own patch to enable checks for
REGRESSION_TEST_NAME_RESTRICTIONS.

-- 
Justin
>From aa4c3ae08b3379bcd222e00aa896a40f811155a5 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 22 Jul 2022 15:52:11 -0500
Subject: [PATCH 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not FILE

WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE

Since the value didn't come from a file.  Or maybe we should have
another PGC_S_ value for this, or a flag for 'is a test'.
---
 src/backend/utils/misc/guc.c | 2 +-
 src/include/utils/guc.h  | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5308896c87f..dda54310a56 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4540,7 +4540,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
 
 			/* Check that it's acceptable for the indicated parameter */
 			if (!parse_and_validate_value(record, name, value,
-		  PGC_S_FILE, ERROR,
+		  PGC_S_TEST, ERROR,
 		  , ))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d5253c7ed23..bd45d40c106 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -119,6 +119,7 @@ typedef enum
 	PGC_S_OVERRIDE,/* special case to forcibly set default */
 	PGC_S_INTERACTIVE,			/* dividing line for error reporting */
 	PGC_S_TEST,	/* test per-database or per-user setting */
+	// PGC_S_TEST_FILE,			/* test global cluster settings (ALTER SYSTEM) */
 	PGC_S_SESSION/* SET command */
 } GucSource;
 
-- 
2.34.1

>From b05c738bddfca11d4d0510c497a121cc8fcb585a Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH 2/4] warn when setting GUC to a nonextant library

Note that warnings shouldn't be issued during startup (only fatals):

$ ./tmp_install/usr/local/pgsql/bin/postgres -D ./testrun/regress/regress/tmp_check/data -c shared_preload_libraries=ab
2023-07-06 14:47:03.817 CDT postmaster[2608106] FATAL:  could not access file "ab": No existe el archivo o el directorio
2023-07-06 14:47:03.817 CDT postmaster[2608106] CONTEXT:  while loading shared libraries for setting "shared_preload_libraries"
---
 

Re: Setting restrictedtoken in pg_regress

[Daniel Gustafsson]

> On 14 Jun 2023, at 13:02, Andrew Dunstan  wrote:
> On 2023-06-12 Mo 19:43, Nathan Bossart wrote:
>> On Tue, Jun 13, 2023 at 08:29:19AM +0900, Michael Paquier wrote:
>> 
>>> I am actually a bit confused with the return value of
>>> CreateRestrictedProcess() on failures in restricted_token.c.  Wouldn't
>>> it be cleaner to return INVALID_HANDLE_VALUE rather than 0 in these
>>> cases?
>>> 
>> My suspicion is that this was chosen to align with CreateProcess and to
>> allow things like
>> 
>>  if (!CreateRestrictedProcess(...))
> 
> Probably, it's been a while. I doubt it's worth changing at this point, and 
> we could just change pg_regress.c to use a boolean test like the above.

Done that way and pushed, thanks!

--
Daniel Gustafsson

Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)

[Ranier Vilela]

Em qui., 6 de jul. de 2023 às 16:06, Gurjeet Singh 
escreveu:

> On Thu, Jul 6, 2023 at 8:01 AM Karina Litskevich
>  wrote:
> >
> >
> >> EB_SMGR and EB_REL are macros for making new structs.
> >> IMO these are buggy, once make new structs without initializing all
> fields.
> >> Attached a patch to fix this and make more clear when rel or smgr is
> NULL.
> >
> >
> > As long as a structure is initialized, its fields that are not present in
> > initialization are initialized to zeros and NULLs depending on their
> types.
> > See C99 Standard 6.7.8.21 and 6.7.8.10. This behaviour is quite well
> known,
> > so I don't think this place is buggy. Anyway, if someone else says the
> code
> > is more readable with these fields initialized explicitly, then go on.
>
> Even though I am not a fan of the Designated Initializers feature, I
> agree with Karina. Per the standard, the unmentioned fields get
> initialized to zeroes/NULLs, so the explicit initialization to
> zero/null that this additional patch does is unnecessary. Moreover, I
> feel that it makes the code less pleasant to read.
>
> C99, 6.7.8.21:
> > If there are fewer initializers in a brace-enclosed list than there are
> > elements or members of an aggregate, or fewer characters in a string
> literal
> > used to initialize an array of known size than there are elements in the
> array,
> > the remainder of the aggregate shall be initialized implicitly the same
> as
> > objects that have static storage duration.
>
> C99, 6.7.8.10:
> > If an object that has automatic storage duration is not initialized
> explicitly,
> > its value is indeterminate.

The key points are here.
The object is not static storage duration.
The object is struct with "automatic storage duration".

And not all compilers follow the standards,
they tend to vary quite a bit.

regards,
Ranier Vilela

Re: [PATCH] Add GitLab CI to PostgreSQL

[Gurjeet Singh]

On Thu, Jul 6, 2023 at 11:27 AM Daniel Gustafsson  wrote:
>
> > On 6 Jul 2023, at 20:10, Gurjeet Singh  wrote:
>
> > I can
> > imagine if cfbot was developed against some other CI, it's very likely
> > that we'd be using that other CI instead of Cirrus.
>
> The CFBot originally used Travis, but switched in late 2020 when Travis almost
> over night become hard to use for open source projects:
>
>   
> https://github.com/macdice/cfbot/commit/a62aa6d77dd4cc7f0a5549db378cd6f1cf25c0e2

Thanks for providing the historical context! A for-profit entity,
despite their best intentions, and sometimes by no fault of their own,
may not survive. It's not that a non-profits are guaranteed to
survive, but the conditions they operate in are drastically different
than those of for-profit ones.

> These systems come and go, and each have their quirks.

I'm sure the community has seen enough of such disappearances over the
years, which is why I was surprised to see the adoption of Cirrus in
core (after I had stopped paying attention to Postgres hackers list
for a few years). Having read that whole discussion, though, I do see
the immense value Cirrus CI provides.

> Having options is good,
> but maintaining multiple ones isn't necessarily a free fire-and-forget type of
> thing for the community.

By not adopting  at least one other CI, it'd seem like the community
is favoring Cirrus over others; and that doesn't feel good.

Best regards,
Gurjeet
http://Gurje.et

Re: Fix last unitialized memory warning

[Andres Freund]

Hi,

On 2023-07-06 10:21:44 +0200, Peter Eisentraut wrote:
> On 05.07.23 23:06, Tristan Partin wrote:
> > Thanks for following up. My system is Fedora 38. I can confirm this is
> > still happening on master.
> > 
> > $ gcc --version
> > gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
> > Copyright (C) 2023 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions.  There is NO
> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > $ meson setup build --buildtype=release
> 
> This buildtype turns on -O3 warnings.  We have usually opted against chasing
> warnings in -O3 level because there are often some false-positive
> uninitialized variable warnings with every new compiler.

OTOH, -O3 is substantially faster IME in cpu bound tests than -O2. It doesn't
seem wise to me for the project to basically say that that's not advisable due
to the level of warnings created.

I've also found bugs with -O3 that -O2 didn't find. And often -O3 warnings end
up showing up with -O2 a compiler major version or three down the line, so
it's often just deferring work.

Greetings,

Andres Freund

Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)

[Gurjeet Singh]

On Thu, Jul 6, 2023 at 8:01 AM Karina Litskevich
 wrote:
>
>
>> EB_SMGR and EB_REL are macros for making new structs.
>> IMO these are buggy, once make new structs without initializing all fields.
>> Attached a patch to fix this and make more clear when rel or smgr is NULL.
>
>
> As long as a structure is initialized, its fields that are not present in
> initialization are initialized to zeros and NULLs depending on their types.
> See C99 Standard 6.7.8.21 and 6.7.8.10. This behaviour is quite well known,
> so I don't think this place is buggy. Anyway, if someone else says the code
> is more readable with these fields initialized explicitly, then go on.

Even though I am not a fan of the Designated Initializers feature, I
agree with Karina. Per the standard, the unmentioned fields get
initialized to zeroes/NULLs, so the explicit initialization to
zero/null that this additional patch does is unnecessary. Moreover, I
feel that it makes the code less pleasant to read.

C99, 6.7.8.21:
> If there are fewer initializers in a brace-enclosed list than there are
> elements or members of an aggregate, or fewer characters in a string literal
> used to initialize an array of known size than there are elements in the 
> array,
> the remainder of the aggregate shall be initialized implicitly the same as
> objects that have static storage duration.

C99, 6.7.8.10:
> If an object that has automatic storage duration is not initialized 
> explicitly,
> its value is indeterminate. If an object that has static storage duration is
> not initialized explicitly, then:
> - if it has pointer type, it is initialized to a null pointer;
> - if it has arithmetic type, it is initialized to (positive or unsigned) zero;
> - if it is an aggregate, every member is initialized (recursively) according 
> to these rules;
> - if it is a union, the first named member is initialized (recursively) 
> according to these rules.


Best regards,
Gurjeet
http://Gurje.et

Re: [PATCH] Add GitLab CI to PostgreSQL

[Andres Freund]

Hi,

On 2023-07-04 23:44:45 +, Newhouse, Robin wrote:
> I propose the attached patch to be applied on the 'master' branch
> of PostgreSQL to add GitLab CI automation alongside Cirrus CI in the 
> PostgreSQL repository.
> 
> It is not intended to be a replacement for Cirrus CI, but simply suggestion 
> for the
> PostgreSQL project to host centrally a Gitlab CI definition for those who 
> prefer to use
> it while developing/testing PostgreSQL.

One way to avoid duplicated CI definition could be to use for gitlab-ci to use
cirrus-cli to run the cirrus CI tests within gitlab ci.

Realistically I think adding a separate CI definition would entail committers
needing to run that CI at least occasionally. If we make the different CI envs
more similar, that becomes less of a necessity.


> +default:
> +  # Base image for builds and tests unless otherwise defined
> +  image: fedora:latest
> +  # Extend build jobs to have longer timeout as the default GitLab
> +  # timeout (1h) is often not enough
> +  timeout: 3h

IMO we shouldn't add CI that doesn't complete within well under an hour, it's
too expensive workflow wise.



> +fedora:
> +  stage: build
> +  variables:
> +GIT_STRATEGY: fetch
> +GIT_SUBMODULE_STRATEGY: normal
> +  script:
> +# Install dependencies
> +- yum install -y yum-utils perl
> +- yum-builddep -y postgresql
> +- *build-postgres-def

My experience is that installing dependencies on each run is way too slow to
be practical. I also found that it often causes temporary failures due to
network issues etc.  For cirrus-ci we create VM and docker images on a regular
schedule (three times a week right now) - if there's interest in building
fedora containers that'd be easy.

I'd be open to switching one of the cirrus-CI tasks over to fedora, fwiw.


> +# From https://github.com/postgres/postgres/blob/master/.cirrus.yml
> +.create-user: 
> +- useradd -m postgres
> +- chown -R postgres:postgres .
> +- mkdir -p ${CCACHE_DIR}
> +- chown -R postgres:postgres ${CCACHE_DIR}
> +- echo '* - memlock 134217728' > /etc/security/limits.d/postgres.conf
> +- su postgres -c "ulimit -l -H && ulimit -l -S"
> +# Can't change container's kernel.core_pattern. Postgres user can't write
> +# to / normally. Change that.
> +- chown root:postgres /
> +- chmod g+rwx /

If we need duplicated stanzas like this, we should instead move them out into
scripts that we can use from different CI environments.


> +# Similar to https://github.com/postgres/postgres/blob/master/.cirrus.yml
> +fedora meson:
> +  stage: build
> +  variables:
> +GIT_STRATEGY: fetch
> +GIT_SUBMODULE_STRATEGY: normal
> +  script:
> +# Meson system only exists on master branch currently

Master and 16 now...


> +- if [ ! -f meson.build ]; then exit 0; fi
> +# Install dependencies
> +- yum install -y yum-utils perl perl-IPC-Run meson ninja-build
> +- yum-builddep -y postgresql
> +# Create postgres user
> +- *create-user-def
> +# Configure
> +- su postgres -c 'meson setup --buildtype=debug --auto-features=disabled 
> -Dtap_tests=enabled build'
> +# Build
> +- su postgres -c 'ninja -C build -j 2'
> +# Minimal test
> +- su postgres -c 'meson test $MTEST_ARGS --num-processes 2 tmp_install 
> cube/regress pg_ctl/001_start_stop'
> +# Run all tests
> +- su postgres -c 'meson test $MTEST_ARGS --num-processes 2'
> +  artifacts:
> +when: always  # Must be able to see logs
> +paths:
> +  - build/meson-logs/testlog.txt

FWIW, that's not enough to be able to debug problems. You really also need the
log files created by failing tests.

Greetings,

Andres Freund

Re: [PATCH] Add GitLab CI to PostgreSQL

[Daniel Gustafsson]

> On 6 Jul 2023, at 20:10, Gurjeet Singh  wrote:

> I can
> imagine if cfbot was developed against some other CI, it's very likely
> that we'd be using that other CI instead of Cirrus.

The CFBot originally used Travis, but switched in late 2020 when Travis almost
over night become hard to use for open source projects:

  
https://github.com/macdice/cfbot/commit/a62aa6d77dd4cc7f0a5549db378cd6f1cf25c0e2

These systems come and go, and each have their quirks.  Having options is good,
but maintaining multiple ones isn't necessarily a free fire-and-forget type of
thing for the community.

--
Daniel Gustafsson

Re: [PATCH] Add support function for containment operators

[Laurenz Albe]

> On Sat, 2023-04-29 at 17:07 +0200, Kim Johan Andersson wrote:
> > I had noticed that performance wasn't great when using the @> or <@ 
> > operators when examining if an element is contained in a range.
> > Based on the discussion in [1] I would like to suggest the following 
> > changes:
> > 
> > This patch attempts to improve the row estimation, as well as opening 
> > the possibility of using a btree index scan when using the containment 
> > operators.

I managed to break the patch:

CREATE DATABASE czech ICU_LOCALE "cs-CZ" LOCALE "cs_CZ.utf8" TEMPLATE template0;

\c czech

CREATE TYPE textrange AS RANGE (SUBTYPE = text, SUBTYPE_OPCLASS = 
text_pattern_ops);

CREATE TABLE tx (t text);

INSERT INTO tx VALUES ('a'), ('c'), ('d'), ('ch');

EXPLAIN SELECT * FROM tx WHERE t <@ textrange('a', 'd');

 QUERY PLAN 

 Seq Scan on tx  (cost=0.00..30.40 rows=7 width=32)
   Filter: ((t >= 'a'::text) AND (t < 'd'::text))
(2 rows)

SELECT * FROM tx WHERE t <@ textrange('a', 'd');
ERROR:  could not determine which collation to use for string comparison
HINT:  Use the COLLATE clause to set the collation explicitly.


The replacement operators are wrong; it should be ~>=~ and ~<~ .

Also, there should be no error message.
The result should be 'a', 'c' and 'ch'.

Yours,
Laurenz Albe

Re: EBCDIC sorting as a use case for ICU rules

[Jeff Davis]

On Thu, 2023-07-06 at 11:32 +0200, Peter Eisentraut wrote:
> CREATE COLLATION ebcdic (provider='icu', locale='und',
> rules=$$
> & ' ' < '.' < '<' < '(' < '+' < \|
> < '&' < '!' < '$' < '*' < ')' < ';'
> < '-' < '/' < ',' < '%' < '_' < '>' < '?'
> < '`' < ':' < '#' < '@' < \' < '=' < '"'
> < a < b < c < d < e < f < g < h < i
> < j < k < l < m < n < o < p < q < r
> < '~' < s < t < u < v < w < x < y < z
> < '[' < '^' < ']'
> < '{' < A < B < C < D < E < F < G < H < I
> < '}' < J < K < L < M < N < O < P < Q < R
> < '\'  < S < T < U < V < W < X < Y < Z
> < 0 < 1 < 2 < 3 < 4 < 5 < 6 < 7 < 8 < 9
> $$);

That looks much nicer and would go nicely in the documentation along
with some explanation.

Regards,
Jeff Davis

Re: [PATCH] Add GitLab CI to PostgreSQL

[Gurjeet Singh]

On Wed, Jul 5, 2023 at 6:22 AM Andrew Dunstan  wrote:
>
>
> On 2023-07-04 Tu 19:44, Newhouse, Robin wrote:
>
> Hello!
>
>
>
> I propose the attached patch to be applied on the 'master' branch
>
> of PostgreSQL to add GitLab CI automation alongside Cirrus CI in the 
> PostgreSQL repository.
>
>
>
> It is not intended to be a replacement for Cirrus CI, but simply suggestion 
> for the
>
> PostgreSQL project to host centrally a Gitlab CI definition for those who 
> prefer to use
>
> it while developing/testing PostgreSQL.
>
>
>
> The intent is to facilitate collaboration among GitLab users, promote 
> standardization
>
> and consistency, and ultimately, improve testing and code quality.
>
>
> This seems very RedHat-centric, which I'm not sure is a good idea.

A few years ago, a proposal to use CentOS may not have been a bad
proposal. But since Redhat changed CentOS to be an upstream distro
(rather than a rebuild of RHEL), I do see a reason to consider RHEL as
a candidate in our CI.

I think the idea of a pre-buildfarm CI is to enable contributors catch
problems before they're merged, or even before proposed as a patch to
the community. So if our CI includes support for a prominent Linux
distro, I think it's worth it, to provide coverage for the large
ecosystem that's based on RHEL and its derivatives.

Would using RockyLinux assuage these concerns? Perhaps, it would.

> If we're going to do this then arguably we should also be supporting GitHub 
> Actions and who knows what other CI frameworks. There is a case for us 
> special casing Cirrus CI because it's used for the cfbot.

We've already lost that battle by supporting one
commercial/non-community provider. From Anrdres' email [1]:

> An obvious criticism of the effort to put CI runner infrastructure into core
> is that they are effectively all proprietary technology, and that we should be
> hesistant to depend too much on one of them. I think that's a valid
> concern. However, once one CI integration is done, a good chunk (but not all!)
> the work is transferrable to another CI solution, which I do think reduces the
> dependency sufficiently.

So it seems that supporting more than one CI was always on the cards.
Cirrus was chosen for its advantages that Andres mentions in the
email, but also for the reason that cfbot had chosen Cirrus. I can
imagine if cfbot was developed against some other CI, it's very likely
that we'd be using that other CI instead of Cirrus.

[1]: 
https://www.postgresql.org/message-id/20211001222752.wrz7erzh4cajvgp6%40alap3.anarazel.de

Best regards,
Gurjeet
http://Gurje.et

Re: Avoid overflow with simplehash

[Ranier Vilela]

Em qui., 6 de jul. de 2023 às 14:33, Andres Freund 
escreveu:

> Hi,
>
> I pushed changing i to uint32 and adding Tom's comment to 11-HEAD.
>
Thank you.

regards,
Ranier Vilela

Re: MERGE ... RETURNING

[Gurjeet Singh]

On Thu, Jul 6, 2023 at 4:07 AM jian he  wrote:
>
> On Thu, Jul 6, 2023 at 1:13 PM Gurjeet Singh  wrote:

> > I think the name of function pg_merge_when_clause() can be improved.
> > How about pg_merge_when_clause_ordinal().
>
> another idea: pg_merge_action_ordinal()

Since there can be many occurrences of the same action
(INSERT/UPDATE/DELETE) in a MERGE command associated with different
conditions, I don't think action_ordinal would make sense for this
function name.

e.g.
WHEN  MATCHED and src.col1 = val1 THEN UPDATE col2 = someval1
WHEN  MATCHED and src.col1 = val2 THEN UPDATE col2 = someval2
...

When looking at the implementation code, as well, we see that the code
in this function tracks and reports the lexical position of the WHEN
clause, irrespective of the action associated with that WHEN clause.

foreach(l, stmt->mergeWhenClauses)
{
...
action->index = foreach_current_index(l) + 1;

Best regards,
Gurjeet
http://Gurje.et

Re: Avoid overflow with simplehash

[Andres Freund]

Hi,

I pushed changing i to uint32 and adding Tom's comment to 11-HEAD.


On 2023-07-06 14:01:55 -0300, Ranier Vilela wrote:
> > > then will it iterate forwards?
> >
> > No, it'd still iterate backwards, but starting from the wrong place - but
> > there is no correct place to start iterating from if there is no unused
> > element.
> >
> Thanks for the confirmation.
> 
> So I suppose we could have this in v1, attached.
> With comments added by Tom.


> diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h
> index 48db837ec8..4fe627a921 100644
> --- a/src/include/lib/simplehash.h
> +++ b/src/include/lib/simplehash.h
> @@ -964,8 +964,8 @@ SH_DELETE_ITEM(SH_TYPE * tb, SH_ELEMENT_TYPE * entry)
>  SH_SCOPE void
>  SH_START_ITERATE(SH_TYPE * tb, SH_ITERATOR * iter)
>  {
> - int i;
> - uint64  startelem = PG_UINT64_MAX;
> + uint32  i;
> + uint32  startelem = PG_UINT32_MAX;

The startelem type change doesn't strike me as a good idea.  Currently
PG_UINT32_MAX is a valid element.

Greetings,

Andres Freund

Re: pgsql: Fix search_path to a safe value during maintenance operations.

[Nathan Bossart]

On Thu, Jul 06, 2023 at 12:55:14AM -0700, Jeff Davis wrote:
> It was difficult to review standalone, so I tried a quick version
> myself and ended up with very similar results.

Thanks for taking a look.

> The only substantial
> difference was that I put back:
> 
> 
> +   if (!vacuum_is_relation_owner(relid, classForm,
> options))
> +   continue;
> 
> 
> in get_all_vacuum_rels() whereas your patch left it out -- double-check
> that we're doing the right thing there.

The privilege check was moved in d46a979, which I think still makes sense,
so I left it there.  That might be why it looks like I removed it.

> Also remember to bump the catversion. Other than that, it looks good to
> me.

Will do.

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

Re: MERGE ... RETURNING

[Gurjeet Singh]

On Thu, Jul 6, 2023 at 3:39 AM Alvaro Herrera  wrote:
>
> On 2023-Jul-05, Gurjeet Singh wrote:

> > I expected the .out file to have captured the stdout. I'm gradually,
> > and gladly, re-learning bits of the test infrastructure.
> >
> > The DELETE command tag in the output does not feel appropriate for a
> > COPY command that's using MERGE as the source of the data.
>
> You misread this one :-)  The COPY output is there, the tag is not.  So
> DELETE is the value from pg_merge_action(), and "1 100" correspond to
> the columns in the the sq_target row that was deleted.  The command tag
> is presumably MERGE 1.

:-) That makes more sense. It matches my old mental model. Thanks for
clarifying!

Best regards,
Gurjeet
http://Gurje.et

Re: Add more sanity checks around callers of changeDependencyFor()

[Andres Freund]

Hi,

On 2023-07-05 14:10:42 +0900, Michael Paquier wrote:
> On Tue, Jul 04, 2023 at 02:40:04PM -0400, Tom Lane wrote:
> > Alvaro Herrera  writes:
> >> Hmm, shouldn't we disallow moving the function to another schema, if the
> >> function's schema was originally determined at extension creation time?
> >> I'm not sure we really want to allow moving objects of an extension to a
> >> different schema.
> > 
> > Why not?  I do not believe that an extension's objects are required
> > to all be in the same schema.
> 
> Yes, I don't see what we would gain by putting restrictions regarding
> which schema an object is located in, depending on which schema an
> extension uses.

Well, it adds an exploitation opportunity. If other functions in the extension
reference the original location (explicitly or via search_path), somebody else
can create a function there, which might be called from a more privileged
context. Obviously permissions limit the likelihood of this being a real
issue.

I also don't think pg_dump will dump the changed schema, which means a
dump/restore leads to a different schema - IMO something to avoid.

Greetings,

Andres Freund

Re: Avoid overflow with simplehash

[Ranier Vilela]

Em qui., 6 de jul. de 2023 às 13:52, Andres Freund 
escreveu:

> Hi,
>
> On 2023-07-06 13:40:00 -0300, Ranier Vilela wrote:
> > I still have doubts about this.
> >
> > see:
> > #include 
> > #include 
> > #include 
> >
> > #define SH_MAX_SIZE1 (((unsigned long long) 0xU) + 1)
> > #define SH_MAX_SIZE2 (((unsigned long long) 0xU) - 1)
> >
> > int main()
> > {
> > unsigned long long max_size1 = SH_MAX_SIZE1;
> > unsigned long long max_size2 = SH_MAX_SIZE2;
> > unsigned int cur1 = SH_MAX_SIZE1;
> > unsigned int cur2 = SH_MAX_SIZE2;
> >
> > printf("SH_MAX_SIZE1=%llu\n", max_size1);
> > printf("SH_MAX_SIZE2=%llu\n", max_size2);
> > printf("cur1=%u\n", cur1);
> > printf("cur2=%u\n", cur2);
> > }
> > warning: implicit conversion from 'unsigned long long' to 'unsigned int'
> > changes value from 4294967296 to 0 [-Wconstant-conversion]
>
> I don't think we at the moment try to not have implicit-conversion warnings
> (nor do we enable them), this would be far from the only place. If we
> wanted
> to here, we'd just need an explicit cast.
>
It was just for show.


>
>
> > outputs:
> > SH_MAX_SIZE1=4294967296
> > SH_MAX_SIZE2=4294967294
> > cur1=0
> > cur2=4294967294
> >
> > And in the comments we have:
> > "Iterate backwards, that allows the current element to be deleted, even
> > * if there are backward shifts"
> >
> > So if an empty element is not found and the *cur* field is set to 0
> > (SH_MAX_SIZE -> uint32),
>
> That should never be reachable - which the assert tries to ensure.
>
Right.


>
>
> > then will it iterate forwards?
>
> No, it'd still iterate backwards, but starting from the wrong place - but
> there is no correct place to start iterating from if there is no unused
> element.
>
Thanks for the confirmation.

So I suppose we could have this in v1, attached.
With comments added by Tom.

regards,
Ranier Vilela


v1-avoid-overflow-with-simplehash-start-iterate.patch
Description: Binary data

Re: [PoC] Federated Authn/z with OAUTHBEARER

[Jacob Champion]

On Wed, Jul 5, 2023 at 3:07 PM Thomas Munro  wrote:
> I guess there are a couple of ways to do it if we give up the goal of
> no-code-change-for-the-client:
>
> 1.  Generalised PQsocket(), that so that a client can call something like:
>
> int PQpollset(const PGConn *conn, struct pollfd fds[], int fds_size,
> int *nfds, int *timeout_ms);
>
> That way, libpq could tell you about which events it would like to
> wait for on which fds, and when it would like you to call it back due
> to timeout, and you can either pass that information directly to
> poll() or WSAPoll() or some equivalent interface (we don't care, we
> just gave you the info you need), or combine it in obvious ways with
> whatever else you want to multiplex with in your client program.

I absolutely wanted something like this while I was writing the code
(it would have made things much easier), but I'd feel bad adding that
much complexity to the API if the vast majority of connections use
exactly one socket. Are there other use cases in libpq where you think
this expanded API could be useful? Maybe to lift some of the existing
restrictions for PQconnectPoll(), add async DNS resolution, or
something?

Couple complications I can think of at the moment:
1. Clients using persistent pollsets will have to remove old
descriptors, presumably by tracking the delta since the last call,
which might make for a rough transition. Bookkeeping bugs probably
wouldn't show up unless they used OAuth in their test suites. With the
current model, that's more hidden and libpq takes responsibility for
getting it right.
2. In the future, we might need to think carefully around situations
where we want multiple PGConn handles to share descriptors (e.g.
multiplexed backend connections). I avoid tricky questions at the
moment by assigning only one connection per multi pool.

> 2.  Convert those events into new libpq events like 'I want you to
> call me back in 100ms', and 'call me back when socket #42 has data',
> and let clients handle that by managing their own poll set etc.  (This
> is something I've speculated about to support more efficient
> postgres_fdw shard query multiplexing; gotta figure out how to get
> multiple connections' events into one WaitEventSet...)

Something analogous to libcurl's socket and timeout callbacks [1],
then? Or is there an existing libpq API you were thinking about using?

> I guess there is a practical middle ground where client code on
> systems that have epoll/kqueue can use OAUTHBEARER without any code
> change, and the feature is available on other systems too but you'll
> have to change your client code to use one of those interfaces or else
> you get an error 'coz we just can't do it.

That's a possibility -- if your platform is able to do it nicely,
might as well use it. (In a similar vein, I'd personally vote against
having every platform use a background thread, even if we decided to
implement it for Windows.)

> Or, more likely in the
> first version, you just can't do it at all...  Doesn't seem that bad
> to me.

Any initial opinions on whether it's worse or better than a worker thread?

> BTW I will happily do the epoll->kqueue port work if necessary.

And I will happily take you up on that; thanks!

--Jacob

[1] https://curl.se/libcurl/c/CURLMOPT_SOCKETFUNCTION.html

Re: Avoid overflow with simplehash

[Andres Freund]

Hi,

On 2023-07-06 13:40:00 -0300, Ranier Vilela wrote:
> I still have doubts about this.
> 
> see:
> #include 
> #include 
> #include 
> 
> #define SH_MAX_SIZE1 (((unsigned long long) 0xU) + 1)
> #define SH_MAX_SIZE2 (((unsigned long long) 0xU) - 1)
> 
> int main()
> {
> unsigned long long max_size1 = SH_MAX_SIZE1;
> unsigned long long max_size2 = SH_MAX_SIZE2;
> unsigned int cur1 = SH_MAX_SIZE1;
> unsigned int cur2 = SH_MAX_SIZE2;
> 
> printf("SH_MAX_SIZE1=%llu\n", max_size1);
> printf("SH_MAX_SIZE2=%llu\n", max_size2);
> printf("cur1=%u\n", cur1);
> printf("cur2=%u\n", cur2);
> }
> warning: implicit conversion from 'unsigned long long' to 'unsigned int'
> changes value from 4294967296 to 0 [-Wconstant-conversion]

I don't think we at the moment try to not have implicit-conversion warnings
(nor do we enable them), this would be far from the only place. If we wanted
to here, we'd just need an explicit cast.


> outputs:
> SH_MAX_SIZE1=4294967296
> SH_MAX_SIZE2=4294967294
> cur1=0
> cur2=4294967294
> 
> And in the comments we have:
> "Iterate backwards, that allows the current element to be deleted, even
> * if there are backward shifts"
> 
> So if an empty element is not found and the *cur* field is set to 0
> (SH_MAX_SIZE -> uint32),

That should never be reachable - which the assert tries to ensure.


> then will it iterate forwards?

No, it'd still iterate backwards, but starting from the wrong place - but
there is no correct place to start iterating from if there is no unused
element.

Greetings,

Andres Freund

Re: Add more sanity checks around callers of changeDependencyFor()

[Peter Eisentraut]

On 29.06.23 01:36, Michael Paquier wrote:

While working on a different patch, I have noted three code paths that
call changeDependencyFor() but don't check that they do not return
errors.  In all the three cases (support function, extension/schema
and object/schema), it seems to me that only one dependency update is
expected.


Why can't changeDependencyFor() raise the error itself?

Re: Avoid overflow with simplehash

[Ranier Vilela]

Em qui., 6 de jul. de 2023 às 12:16, Tom Lane  escreveu:

> Ranier Vilela  writes:
> > See the comments:
> > "Search for the first empty element."
> > If the empty element is not found, startelem has PG_UINT64_MAX value,
> > which do not fit in uint32.
>
> Hi Tom,

> I think the point of that assertion is exactly that we're required to
> have an empty element (because max fillfactor is less than 1),
> so the search should have succeeded.
>
> It does seem like we could do
>
> uint64  startelem = SH_MAX_SIZE;
>
> ...
>
> Assert(startelem < SH_MAX_SIZE);
>
> which'd make it a little clearer that the expectation is for
> startelem to have changed value.

I still have doubts about this.

see:
#include 
#include 
#include 

#define SH_MAX_SIZE1 (((unsigned long long) 0xU) + 1)
#define SH_MAX_SIZE2 (((unsigned long long) 0xU) - 1)

int main()
{
unsigned long long max_size1 = SH_MAX_SIZE1;
unsigned long long max_size2 = SH_MAX_SIZE2;
unsigned int cur1 = SH_MAX_SIZE1;
unsigned int cur2 = SH_MAX_SIZE2;

printf("SH_MAX_SIZE1=%llu\n", max_size1);
printf("SH_MAX_SIZE2=%llu\n", max_size2);
printf("cur1=%u\n", cur1);
printf("cur2=%u\n", cur2);
}
warning: implicit conversion from 'unsigned long long' to 'unsigned int'
changes value from 4294967296 to 0 [-Wconstant-conversion]

outputs:
SH_MAX_SIZE1=4294967296
SH_MAX_SIZE2=4294967294
cur1=0
cur2=4294967294

And in the comments we have:
"Iterate backwards, that allows the current element to be deleted, even
* if there are backward shifts"

So if an empty element is not found and the *cur* field is set to 0
(SH_MAX_SIZE -> uint32),
then will it iterate forwards?

  And I agree that declaring "i"
> as int is wrong.
>
Thanks for the confirmation.

regards,
Ranier Vilela

Re: UUID v7

[Peter Eisentraut]

On 06.07.23 16:02, Tom Lane wrote:

Daniel Gustafsson  writes:

On 6 Jul 2023, at 15:29, Matthias van de Meent  
wrote:

Sure, it's earlier than the actual release of
the standard, but that wasn't a blocker for SQL features that were
considered finalized either.



I can't speak for any SQL standard features we've committed before being
standardized, it's for sure not the norm for the project.


We have done a couple of things that way recently.  An important
reason why we felt we could get away with that is that nowadays
we have people who actually sit on the SQL committee and have
reliable information on what's likely to make it into the final text
of the next version.  I don't think we have equivalent visibility or
should have equivalent confidence about how UUID v7 standardization
will play out.


(I have been attending some meetings and I'm on the mailing list.)

Anyway, I think it would be reasonable to review this patch now.  We 
might leave it hanging in "Ready for Committer" for a while when we get 
there.  But surely review can start now.

Re: pg_basebackup check vs Windows file path limits

[Andrew Dunstan]

On 2023-07-06 Th 09:50, Daniel Gustafsson wrote:

On 5 Jul 2023, at 14:49, Andrew Dunstan  wrote:
On 2023-07-04 Tu 16:54, Daniel Gustafsson wrote:

On 4 Jul 2023, at 20:19, Andrew Dunstan
  wrote:

But sadly we're kinda back where we started. fairywren is failing on REL_16_STABLE. 
Before the changes the failure occurred because the test script was unable to create 
the file with a path > 255. Now that we have a way to create the file the test for 
pg_basebackup to reject files with names > 100 fails, I presume because the server 
can't actually see the file. At this stage I'm thinking the best thing would be to 
skip the test altogether on windows if the path is longer than 255.


That does sound like a fairly large hammer for a nail small enough that we
should be able to fix it, but I don't have any other good ideas off the cuff.

Not sure it's such a big hammer. Here's a patch.

No objections to the patch, LGTM.



Thanks. pushed with a couple of tweaks.


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: check_strxfrm_bug()

[Peter Eisentraut]

On 05.07.23 00:15, Thomas Munro wrote:

On Tue, Jul 4, 2023 at 2:52 AM Tristan Partin  wrote:

The patch looks good to me as well. Happy to rebase my other patch on
this one.


Thanks.  Here is a slightly tidier version.  It passes on CI[1]
including the optional extra MinGW64/Meson task, and the
MinGW64/autoconf configure+build that is in the SanityCheck task.
There are two questions I'm hoping to get feedback on:  (1) I believe
that defining HAVE_MBSTOWCS_L etc in win32_port.h is the best idea
because that is also where we define mbstowcs_l() etc.  Does that make
sense?  (2) IIRC, ye olde Solution.pm system might break if I were to
completely remove HAVE_MBSTOWCS_L and HAVE_WCSTOMBS_L from Solution.pm
(there must be a check somewhere that compares it with pg_config.h.in
or something like that), but it would also break if I defined them as
1 there (macro redefinition).


I think the correct solution is to set HAVE_MBSTOWCS_L in Solution.pm. 
Compare HAVE_FSEEKO, which is set in Solution.pm with fseeko being 
defined in win32_port.h.

Re: [PATCH] Add support function for containment operators

[Laurenz Albe]

On Sat, 2023-04-29 at 17:07 +0200, Kim Johan Andersson wrote:
> I had noticed that performance wasn't great when using the @> or <@ 
> operators when examining if an element is contained in a range.
> Based on the discussion in [1] I would like to suggest the following 
> changes:
> 
> This patch attempts to improve the row estimation, as well as opening 
> the possibility of using a btree index scan when using the containment 
> operators.
> 
> This is done via a new support function handling the following 2 requests:
> 
> * SupportRequestIndexCondition
> find_index_quals will build an operator clause, given at least one 
> finite RangeBound.
> 
> * SupportRequestSimplify
> find_simplified_clause will rewrite the containment operator into a 
> clause using inequality operators from the btree family (if available 
> for the element type).
> 
> A boolean constant is returned if the range is either empty or has no 
> bounds.
> 
> Performing the rewrite here lets the clausesel machinery provide the 
> same estimates as for normal scalar inequalities.
> 
> In both cases build_bound_expr is used to build the operator clauses 
> from RangeBounds.

I think that this is a small, but useful improvement.

The patch applies and builds without warning and passes "make 
installcheck-world"
with the (ample) new regression tests.

Some comments:

- About the regression tests:
  You are using EXPLAIN (ANALYZE, SUMMARY OFF, TIMING OFF, COSTS OFF).
  While that returns stable results, I don't see the added value.
  I think you should use EXPLAIN (COSTS OFF).  You don't need to test the
  actual number of result rows; we can trust that the executor  processes
  >= and < correctly.
  Plain EXPLAIN would speed up the regression tests, which is a good thing.

- About the implementation:
  You implement both "SupportRequestIndexCondition" and 
"SupportRequestSimplify",
  but when I experimented, the former was never called.  That does not
  surprise me, since any expression of the shape "expr <@ range constant"
  can be simplified.  Is the "SupportRequestIndexCondition" branch dead code?
  If not, do you have an example that triggers it?

- About the code:

  +static Node *
  +find_index_quals(Const *rangeConst, Expr *otherExpr, Oid opfamily)
  +{
  [...]
  +
  +   if (!(lower.infinite && upper.infinite))
  +   {
 [...]
  +   }
  +
  +   return NULL;

  To avoid deep indentation and to make the code more readable, I think
  it would be better to write

  if (!(lower.infinite && upper.infinite))
  return NULL;

  and unindent the rest of the code


  +static Node *
  +match_support_request(Node *rawreq)
  +{
  [...]
  +   switch (req->funcid)
  +   {
  +   case F_ELEM_CONTAINED_BY_RANGE:
  [...]
  +   case F_RANGE_CONTAINS_ELEM:
  [...]
  +   default:
  +   return NULL;
  +   }

  (This code appears twice.)

  The default clause should not be reachable, right?
  I think that there should be an Assert() to verify that.
  Perhaps something like

  Assert(req->funcid == F_ELEM_CONTAINED_BY_RANGE ||
 req->funcid == F_RANGE_CONTAINS_ELEM);

  if (req->funcid == F_ELEM_CONTAINED_BY_RANGE)
  {
  [...]
  }
  else if (req->funcid == F_RANGE_CONTAINS_ELEM)
  {
  [...]
  }

Yours,
Laurenz Albe

Re: Does a cancelled REINDEX CONCURRENTLY need to be messy?

[Peter Eisentraut]

On 03.07.23 19:46, Álvaro Herrera wrote:

Well, I definitely agree that it would be useful to have*something*
that automatically removes debris


Yeah, like "undo".

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

[Daniel Gustafsson]

This patch no longer applies to master, please submit a rebased version to the
thread. I've marked the CF entry as waiting for author in the meantime.

--
Daniel Gustafsson

Re: Reducing connection overhead in pg_upgrade compat check phase

[Daniel Gustafsson]

> On 4 Jul 2023, at 21:08, Nathan Bossart  wrote:
> 
> I put together a rebased version of the patch for cfbot.

Thanks for doing that, much appreciated!  I was busy looking at other peoples
patches and hadn't gotten to my own yet =)

> On 13 Mar 2023, at 19:21, Nathan Bossart  wrote:

> I noticed that git-am complained when I applied the patch:
> 
>Applying: pg_upgrade: run all data type checks per connection
>.git/rebase-apply/patch:1023: new blank line at EOF.
>+
>warning: 1 line adds whitespace errors.

Fixed.

> + for (int rowno = 0; rowno < ntups; rowno++)
> + {
> + found = true;
> 
> It looks like "found" is set unconditionally a few lines above, so I think
> this is redundant.

Correct, this must've been a leftover from a previous coding that changed.
Removed.

> Also, I think it would be worth breaking check_for_data_types_usage() into
> a few separate functions (or doing some other similar refactoring) to
> improve readability.  At this point, the function is quite lengthy, and I
> count 6 levels of indentation at some lines.


It it is pretty big for sure, but it's also IMHO not terribly complicated as
it's not really performing any hard to follow logic.

I have no issues refactoring it, but trying my hand at I was only making (what
I consider) less readable code by having to jump around so I consider it a
failure.  If you have any suggestions, I would be more than happy to review and
incorporate those though.

Attached is a v5 with the above fixes and a pgindenting to fix up a few runaway
comments and indentations.

--
Daniel Gustafsson



v5-0001-pg_upgrade-run-all-data-type-checks-per-connectio.patch
Description: Binary data

Re: pgindent vs. pgperltidy command-line arguments

[Andrew Dunstan]

On 2023-06-21 We 07:35, Andrew Dunstan wrote:



On 2023-06-21 We 05:09, Peter Eisentraut wrote:

On 20.06.23 17:38, Andrew Dunstan wrote:

+1, although I wonder if we shouldn't follow pgindent's new lead
and require some argument(s).


That makes sense to me.  Here is a small update with this behavior 
change and associated documentation update.


I'm intending to add some of the new pgindent features to 
pgperltidy. Preparatory to that here's a rewrite of pgperltidy in 
perl - no new features yet but it does remove the hardcoded path, 
and requires you to pass in one or more files / directories as 
arguments.


Are you planning to touch pgperlcritic and pgperlsyncheck as well? 



Yeah, it would make sense to.




Here's a patch that turns all these into perl scripts.


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/src/tools/perlcheck/FindPerlFiles.pm b/src/tools/perlcheck/FindPerlFiles.pm
new file mode 100644
index 00..3a098f6614
--- /dev/null
+++ b/src/tools/perlcheck/FindPerlFiles.pm
@@ -0,0 +1,62 @@
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+=pod
+
+=head1 NAME
+
+FindPerlFiles - module for finding perl files from a list of paths
+
+=head1 SYNOPSIS
+
+  use FindPerlFiles;
+
+  my @files = FindPerlFiles::findperl(path, ...);
+
+
+=head1 DESCRIPTION
+
+FindPerlFiles finds files which either have a perl extension (.pl or .pm) or
+are bot executable and found by the `file` program to be perl scripts.
+
+=cut
+
+
+package FindPerlFiles;
+
+use strict;
+use warnings;
+
+use File::Find;
+use File::stat;
+use Fcntl ':mode';
+
+my @files;
+
+sub _is_perl_exec
+{
+	my $name = shift;
+	my $out = `file $name 2>/dev/null`;
+	return $out =~ /:.*perl[0-9]*\b/i;
+}
+
+sub findperl
+{
+	my @files ;
+	my $wanted = sub
+	{
+		my $name = $File::Find::name;
+		my $st;
+		# check it's a plain file and either it has a perl extension (.p[lm])
+		# or it's executable and `file` thinks it's a perl script.
+		($st = lstat($_))
+		  && -f $st
+		  && (/\.p[lm]$/ || ((($st->mode & S_IXUSR) && _is_perl_exec($_
+		  && push(@files, $name);
+	};
+
+	File::Find::find({ wanted => $wanted }, @_);
+	return @files;
+}
+
+1;
diff --git a/src/tools/perlcheck/find_perl_files b/src/tools/perlcheck/find_perl_files
deleted file mode 100644
index 20dceb800d..00
--- a/src/tools/perlcheck/find_perl_files
+++ /dev/null
@@ -1,18 +0,0 @@
-# src/tools/perlcheck/find_perl_files
-
-# shell function to find all perl files in the source tree
-
-find_perl_files () {
-	if [ $# -eq 0 ]; then
-		echo 'No files to process' 1>&2
-		return
-	fi
-{
-		# take all .pl and .pm files
-		find "$@" -type f -name '*.p[lm]' -print
-		# take executable files that file(1) thinks are perl files
-		find "$@" -type f -perm -100 -exec file {} \; -print |
-		egrep -i ':.*perl[0-9]*\>' |
-		cut -d: -f1
-	} | sort -u | grep -v '^\./\.git/'
-}
diff --git a/src/tools/perlcheck/pgperlcritic b/src/tools/perlcheck/pgperlcritic
index 2ec6f20de3..87d5e92df7 100755
--- a/src/tools/perlcheck/pgperlcritic
+++ b/src/tools/perlcheck/pgperlcritic
@@ -1,20 +1,27 @@
-#!/bin/sh
+#!/usr/bin/perl
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
 
 # src/tools/perlcheck/pgperlcritic
 
-test -f src/tools/perlcheck/perlcriticrc || {
-	echo could not find src/tools/perlcheck/perlcriticrc
-	exit 1
-	}
+use strict;
+use warnings;
 
-set -e
+use FindBin ();
+use lib "$FindBin::Bin";
+use FindPerlFiles;
+
+die "No directories or files specified\n" unless @ARGV;
+
+my $rc_loc = "$FindBin::Bin/perlcriticrc";
+
+die "no $rc_loc\n" unless -f $rc_loc;
 
 # set this to override default perlcritic program:
-PERLCRITIC=${PERLCRITIC:-perlcritic}
+my $perlcritic = $ENV{PERLCRITIC} || 'perlcritic';
 
-. src/tools/perlcheck/find_perl_files
+my @files = FindPerlFiles::findperl(@ARGV);
 
-find_perl_files "$@" | xargs $PERLCRITIC \
-	  --quiet \
-	  --program-extensions .pl \
-	  --profile=src/tools/perlcheck/perlcriticrc
+exit unless @files;
+
+exec $perlcritic, '--program-extensions',  '.pl', "--profile=$rc_loc", @files
diff --git a/src/tools/perlcheck/pgperlsyncheck b/src/tools/perlcheck/pgperlsyncheck
index da59c9727c..70ff248daf 100755
--- a/src/tools/perlcheck/pgperlsyncheck
+++ b/src/tools/perlcheck/pgperlsyncheck
@@ -1,16 +1,48 @@
-#!/bin/sh
+#!/usr/bin/perl
+
+# Copyright (c) 2023, PostgreSQL Global Development Group
 
 # script to detect compile time errors and warnings in all perl files
 
-INCLUDES="-I src/tools/msvc -I src/tools/msvc/dummylib -I src/backend/catalog"
-INCLUDES="-I src/test/perl -I src/backend/utils/mb/Unicode $INCLUDES"
-INCLUDES="-I src/bin/pg_rewind -I src/test/ssl/t $INCLUDES"
+use strict;
+use warnings;
 
-set -e
+use FindBin ();
+use lib "$FindBin::Bin";
 
-. src/tools/perlcheck/find_perl_files
+use Cwd qw(abs_path);
+use FindPerlFiles;
 
-# for zsh
-setopt shwordsplit 2>/dev/null || true
+die "No directories or files specified\n" unless @ARGV;
 
-find_perl_files "$@" | xargs -L 1 perl 

Re: Avoid overflow with simplehash

[Tom Lane]

Andres Freund  writes:
> On 2023-07-06 11:16:26 -0400, Tom Lane wrote:
>> It does seem like we could do
>>  uint64  startelem = SH_MAX_SIZE;
>>  ...
>>  Assert(startelem < SH_MAX_SIZE);
>> which'd make it a little clearer that the expectation is for
>> startelem to have changed value.

> I guess? I find it easier to understand all-bits-set in a coredump as
> too-large than SH_MAX_SIZE, but ...

What'd help even more is a comment:

/* We should have found an empty element */
Assert(startelem < SH_MAX_SIZE);

regards, tom lane

Re: Problems with estimating OR conditions, IS NULL on LEFT JOINs

[Tomas Vondra]

On 7/6/23 15:51, Alena Rybakina wrote:
> Hi, all!
> 
> On 26.06.2023 12:22, Andrey Lepikhov wrote:
>> On 24/6/2023 17:23, Tomas Vondra wrote:
>>> I really hope what I just wrote makes at least a little bit of sense.
>> Throw in one more example:
>>
>> SELECT i AS id INTO l FROM generate_series(1,10) i;
>> CREATE TABLE r (id int8, v text);
>> INSERT INTO r (id, v) VALUES (1, 't'), (-1, 'f');
>> ANALYZE l,r;
>> EXPLAIN ANALYZE
>> SELECT * FROM l LEFT OUTER JOIN r ON (r.id = l.id) WHERE r.v IS NULL;
>>
>> Here you can see the same kind of underestimation:
>> Hash Left Join  (... rows=500 width=14) (... rows=9 ...)
>>
>> So the eqjoinsel_unmatch_left() function should be modified for the
>> case where nd1>
>>
>> Unfortunately, this patch could not fix the cardinality calculation in
>> this request, I'll try to look and figure out what is missing here.
> 
> I tried to fix the cardinality score in the query above by changing:
> 
> diff --git a/src/backend/utils/adt/selfuncs.c
> b/src/backend/utils/adt/selfuncs.c
> index 8e18aa1dd2b..40901836146 100644
> --- a/src/backend/utils/adt/selfuncs.c
> +++ b/src/backend/utils/adt/selfuncs.c
> @@ -2604,11 +2604,16 @@ eqjoinsel_inner(Oid opfuncoid, Oid collation,
>  * if we're calculating fraction of NULLs or fraction of
> unmatched rows.
>  */
>     // unmatchfreq = (1.0 - nullfrac1) * (1.0 - nullfrac2);
> -   if (nd1 > nd2)
> +   if (nd1 != nd2)
>     {
> -   selec /= nd1;
> -   *unmatched_frac = (nd1 - nd2) * 1.0 / nd1;
> +   selec /= Max(nd1, nd2);
> +   *unmatched_frac = abs(nd1 - nd2) * 1.0 /
> Max(nd1, nd2);
>     }
> +   /*if (nd1 > nd2)
> +   {
> +   selec /= nd1;
> +   *unmatched_frac = nd1 - nd2 * 1.0 / nd1;
> +   }*/
>     else
>     {
>     selec /= nd2;
> 
> and it worked:
> 
> SELECT i AS id INTO l FROM generate_series(1,10) i;
> CREATE TABLE r (id int8, v text);
> INSERT INTO r (id, v) VALUES (1, 't'), (-1, 'f');
> ANALYZE l,r;
> EXPLAIN ANALYZE
> SELECT * FROM l LEFT OUTER JOIN r ON (r.id = l.id) WHERE r.v IS NULL;
> ERROR:  relation "l" already exists
> ERROR:  relation "r" already exists
> INSERT 0 2
> ANALYZE
>   QUERY
> PLAN   
> ---
>  Hash Left Join  (cost=1.09..1944.13 rows=8 width=14) (actual
> time=0.152..84.184 rows=9 loops=1)
>    Hash Cond: (l.id = r.id)
>    Filter: (r.v IS NULL)
>    Rows Removed by Filter: 2
>    ->  Seq Scan on l  (cost=0.00..1443.00 rows=10 width=4) (actual
> time=0.040..27.635 rows=10 loops=1)
>    ->  Hash  (cost=1.04..1.04 rows=4 width=10) (actual time=0.020..0.022
> rows=4 loops=1)
>  Buckets: 1024  Batches: 1  Memory Usage: 9kB
>  ->  Seq Scan on r  (cost=0.00..1.04 rows=4 width=10) (actual
> time=0.009..0.011 rows=4 loops=1)
>  Planning Time: 0.954 ms
>  Execution Time: 92.309 ms
> (10 rows)
> 
> It looks too simple and I suspect that I might have missed something
> somewhere, but so far I haven't found any examples of queries where it
> doesn't work.
> 
> I didn't see it breaking anything in the examples from my previous
> letter [1].
> 

I think it's correct. Or at least it doesn't break anything my patch
didn't already break. My patch was simply written for one specific
query, so it didn't consider the option that the nd1 and nd2 values
might be in the opposite direction ...

> 1.
> https://www.postgresql.org/message-id/7af1464e-2e24-cfb1-b6d4-1544757f8cfa%40yandex.ru
> 
> 
> Unfortunately, I can't understand your idea from point 4, please explain it?
> 
> The good thing is this helps even for IS NULL checks on non-join-key
> columns (where we don't switch to an antijoin), but there's a couple
> things that I dislike ...
> 
> 1) It's not restricted to outer joins or anything like that (this is
> mostly just my laziness / interest in one particular query, but also
> something the outer-join-aware patch might help with).
> 
> 2) We probably don't want to pass this kind of information through
> sjinfo. That was the simplest thing for an experimental patch, but I
> suspect it's not the only piece of information we may need to pass to
> the lower levels of estimation code.
> 
> 3) I kinda doubt we actually want to move this responsibility (to
> consider fraction of unmatched rows) to the low-level estimation
> routines (e.g. nulltestsel and various others). AFAICS this just
> "introduces NULLs" into the relation, so maybe we could "adjust" the
> attribute statistics (in examine_variable?) by inflating null_frac and
> modifying the other frequencies in MCV/histogram.
> 
> 4) But I'm not 

Re: Avoid unused value (src/fe_utils/print.c)

[Karina Litskevich]

>
>
>
>> But
>> it's not obvious, so I also have doubts about removing this line. If
>> someday
>> print options are changed, for example to support printing footers and not
>> printing headers, or anything else changes in this function, the output
>> might
>> be unexpected with this line removed.
>
>
> That part I didn't understand.
> How are we going to make this function less readable by removing the
> complicating part.
>

My point is, technically right now you won't see any difference in output
if you remove the line. Because if we get to that line the need_recordsep
is already true. However, understanding why it is true is complicated.
That's
why if you remove the line people who read the code will wonder why we don't
need a separator after "fputs"ing a footer. So keeping that line will make
the code more readable.
Moreover, removing the line will possibly complicate the future maintenance.
As I wrote in the part you just quoted, if the function changes in the way
that need_recordsep is not true right before printing footers any more, then
output will be unexpected.

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/

Re: pg_recvlogical prints bogus error when interrupted

[Tristan Partin]

On Thu Apr 27, 2023 at 12:54 AM CDT, Bharath Rupireddy wrote:
> Thanks for reviewing. I removed the (void) casts like elsewhere in the
> code, however, I didn't change such casts in prepareToTerminate() to
> not create a diff.
>
> I'm attaching the v4 patch for further review.

Bharath,

I signed up to review the patch for the commitfest. The patch looks
pretty good to me, but I would like to come to a conclusion on what
Andres posted earlier in the thread.

> Why do we need both time_to_abort and ready_to_exit?

I am trying to understand why we need both as well. Maybe I am missing
something important :).

> /* It is not unexepected termination error when Ctrl-C'ed. */

My only other comment is that it would be nice to have the word "an"
before unexpected.

-- 
Tristan Partin
Neon (https://neon.tech)

Re: Avoid overflow with simplehash

[Andres Freund]

Hi,

On 2023-07-06 11:16:26 -0400, Tom Lane wrote:
> Ranier Vilela  writes:
> > See the comments:
> > "Search for the first empty element."
> > If the empty element is not found, startelem has PG_UINT64_MAX value,
> > which do not fit in uint32.
> 
> I think the point of that assertion is exactly that we're required to
> have an empty element (because max fillfactor is less than 1),
> so the search should have succeeded.

Right, that part of the proposed change seems bogus to me.


> It does seem like we could do
> 
>   uint64  startelem = SH_MAX_SIZE;
> 
>   ...
> 
>   Assert(startelem < SH_MAX_SIZE);
> 
> which'd make it a little clearer that the expectation is for
> startelem to have changed value.

I guess? I find it easier to understand all-bits-set in a coredump as
too-large than SH_MAX_SIZE, but ...


> And I agree that declaring "i" as int is wrong.

Yea, that's definitely not right, not sure how I ended up with that. Will push
a fix. I guess it should be backpatched...

Greetings,

Andres Freund

Re: [PATCH] Add GitLab CI to PostgreSQL

[Peter Eisentraut]

On 06.07.23 13:32, Andrew Dunstan wrote:

This seems very RedHat-centric, which I'm not sure is a good idea. Also, 
shouldn't at least some of these recipes call dnf and dnf-builddep instead of 
yum and yum-build-dep?

I don't think it's bad to add an automated test suite for redhat-based images?


I didn't suggest it wasn't just that the coverage should be broader.


If we were to accept this (or other providers besides Cirrus), then I 
think they should run the exact same configurations that we have for 
Cirrus right now (or possibly subsets or supersets, depending on 
availability and capabilities).  Those have been put there with a lot of 
care to get efficient and reasonably broad coverage.  There is no point 
in starting that whole journey over again.


If someone thinks we should have more coverage for Red Hat-based 
platforms, then let's put that into the Cirrus configuration.  That 
should be independent of the choice of CI provider.

Re: Avoid overflow with simplehash

[Tom Lane]

Ranier Vilela  writes:
> See the comments:
> "Search for the first empty element."
> If the empty element is not found, startelem has PG_UINT64_MAX value,
> which do not fit in uint32.

I think the point of that assertion is exactly that we're required to
have an empty element (because max fillfactor is less than 1),
so the search should have succeeded.

It does seem like we could do

uint64  startelem = SH_MAX_SIZE;

...

Assert(startelem < SH_MAX_SIZE);

which'd make it a little clearer that the expectation is for
startelem to have changed value.  And I agree that declaring "i"
as int is wrong.

regards, tom lane

Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL

[Önder Kalacı]

Hi Hayato,


> BTW, I have doubt that the restriction is not related with your commit.
> In other words, if the table has attributes which the datatype is not for
> operator
> class of Btree, we could not use REPLICA IDENTITY FULL. IIUC it is not
> documented.
> Please see attched script to reproduce that. The final DELETE statement
> cannot be
> replicated at the subscriber on my env.
>
>
Yes, I agree, it is (and was before my patch as well)
un-documented limitation of REPLICA IDENTITY FULL.
And, as far as I can see, my patch actually didn't have any impact on the
limitation. The unsupported
cases are still unsupported, but now the same error is thrown in a slightly
different place.

I think that is a minor limitation, but maybe should be listed [1]?

>
> For the specific notes you raised about strategy numbers / operator
> classes, I need to
> study a bit :) Though, I'll be available to do that early next week.
> >
>
> Thanks! I'm looking forward to see your opinions...
>

For this one, I did some research in the code, but  I'm not very
comfortable with the answer. Still, I wanted to share my observations so
that it might be useful for the discussion.

First, I checked if the function get_op_btree_interpretation() could be
used here. But, I think that is again btree-only and I couldn't find
anything generic that does something similar.

Then, I tried to come up with a SQL query, actually based on the link [2]
you shared. I think we should always have an "equality" strategy (e.g.,
"same", "overlaps", "contains" etc sounds wrong to me).

And, it seems btree, hash and brin supports "equal". So, a query like the
following seems to provide the list of (index type, strategy_number,
data_type) that we might be allowed to use.

  SELECT
am.amname AS index_type,
 amop.amoplefttype::regtype,amop.amoprighttype::regtype,
op.oprname AS operator,
amop.amopstrategy AS strategy_number
FROM
pg_amop amop
JOIN
pg_am am ON am.oid = amop.amopmethod
JOIN
pg_operator op ON op.oid = amop.amopopr
WHERE
(am.amname = 'btree' and amop.amopstrategy = 3) OR
(am.amname = 'hash' and amop.amopstrategy = 1) OR
(am.amname = 'brin' and amop.amopstrategy = 3)
ORDER BY
index_type,
strategy_number;


What do you think?


[1]
https://www.postgresql.org/docs/current/logical-replication-restrictions.html

[2] https://www.postgresql.org/docs/devel/xindex.html#XINDEX-STRATEGIES

Re: Avoid overflow with simplehash

[Ranier Vilela]

Em qui., 6 de jul. de 2023 às 12:00, Daniel Gustafsson 
escreveu:

> > On 6 Jul 2023, at 16:42, Ranier Vilela  wrote:
> > Em qui., 6 de jul. de 2023 às 11:37, Daniel Gustafsson  > escreveu:
>
> > #define SH_MAX_SIZE (((uint64) PG_UINT32_MAX) + 1)
> > This is Assert, that is, in production this test is not done.
>
> Correct, which implies that it's a test for something which is deemed
> highly
> unlikely to happen in production.
>
 Highly improbable does not mean impossible, or that it will never happen.


> > If the empty element is not found, startelem has PG_UINT64_MAX value,
> > which do not fit in uint32.
>
> Can you show an example where the hash isn't grown automatically to
> accomodate
> this such that the assertion is tripped?
>
A demo won't change the fact that the function can fail, even if it isn't
currently failing.
As a precaution to avoid future bugs, I think it's necessary to apply the
patch to increase the robustness of the function.

regards,
Ranier Vilela

Re: Avoid overflow with simplehash

[Daniel Gustafsson]

> On 6 Jul 2023, at 16:42, Ranier Vilela  wrote:
> Em qui., 6 de jul. de 2023 às 11:37, Daniel Gustafsson  > escreveu:

> #define SH_MAX_SIZE (((uint64) PG_UINT32_MAX) + 1)
> This is Assert, that is, in production this test is not done.

Correct, which implies that it's a test for something which is deemed highly
unlikely to happen in production.

> If the empty element is not found, startelem has PG_UINT64_MAX value, 
> which do not fit in uint32.

Can you show an example where the hash isn't grown automatically to accomodate
this such that the assertion is tripped?

--
Daniel Gustafsson

Re: Example Table AM implementation

[Hannu Krosing]

Thanks a lot Mark,

I will take a look at this and get back to you if I find anything unclear

---
Hannu

On Tue, Jul 4, 2023 at 10:14 PM Mark Dilger
 wrote:
>
> Hackers,
>
> Over in [1], Hannu Krosing asked me to create and post several Table Access 
> Methods for testing/example purposes.  I am fairly happy to do so, but each 
> one is large, and should be considered separately for inclusion/rejection in 
> contrib/, or in src/test/modules as Michael Paquier suggests.  As such, I am 
> starting this new email thread for the first such TAM.  I've named it "pile", 
> which is an English synonym of "heap", and which is also four characters in 
> length, making for easier side-by-side diffs with the heap code.  The pile 
> code is a deep copy of the heap code, meaning that pile functions do not call 
> heap code, nor run the in-core regression tests, but rather pile's own 
> modified copy of the heap code, the regression tests, and even the test data. 
>  Rather than creating a bare-bones skeleton which needs to be populated with 
> an implementation and regression tests, this patch instead offers a fully 
> fleshed out TAM which can be pruned down to something reasonably compact once 
> the user changes it into whatever they want it to be.  To reiterate, the 
> patch is highly duplicative of in-core files.
>
> Hannu, I'm happy to post something like this three times again, for the named 
> TAMs you request, but could you first review this patch and maybe try turning 
> it into something else, such as the in memory temp tables, overlay tables, or 
> python based tables that you mentioned in [1]?  Anything that needs to be 
> changed to make similar TAMs suitable for the community should be discussed 
> prior to spamming -hackers with more TAMs.  Thanks.
>
>
> [1] 
> https://www.postgresql.org/message-id/CAMT0RQQXtq8tgVPdFb0mk4v%2BcVuGvPWk1Oz9LDr0EgBfrV6e6w%40mail.gmail.com
>
>
>
> —
> Mark Dilger
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
>

Re: Avoid overflow with simplehash

[Ranier Vilela]

Em qui., 6 de jul. de 2023 às 11:37, Daniel Gustafsson 
escreveu:

> > On 6 Jul 2023, at 16:28, Ranier Vilela  wrote:
>
> > The function SH_START_ITERATE can trigger some overflow.
> >
> > See:
> > typedef struct SH_ITERATOR
> > {
> > uint32 cur; /* current element */
> > uint32 end;
> > bool done; /* iterator exhausted? */
> > } SH_ITERATOR;
> >
> > The cur field is uint32 size and currently can be stored a uint64,
> > which obviously does not fit.
>
> -   Assert(startelem < SH_MAX_SIZE);
> +   Assert(startelem < PG_UINT32_MAX);
>
> I mighe be missing something, but from skimming the current code,
> SH_MAX_SIZE
> is currently defined as:
>
> #define SH_MAX_SIZE (((uint64) PG_UINT32_MAX) + 1)
>
This is Assert, that is, in production this test is not done.

See the comments:
"Search for the first empty element."

If the empty element is not found, startelem has PG_UINT64_MAX value,
which do not fit in uint32.

Can you see this?

regards,
Ranier Vilela

Re: Avoid overflow with simplehash

[Daniel Gustafsson]

> On 6 Jul 2023, at 16:28, Ranier Vilela  wrote:

> The function SH_START_ITERATE can trigger some overflow.
> 
> See:
> typedef struct SH_ITERATOR
> {
> uint32 cur; /* current element */
> uint32 end;
> bool done; /* iterator exhausted? */
> } SH_ITERATOR;
> 
> The cur field is uint32 size and currently can be stored a uint64,
> which obviously does not fit.

-   Assert(startelem < SH_MAX_SIZE);
+   Assert(startelem < PG_UINT32_MAX);

I mighe be missing something, but from skimming the current code, SH_MAX_SIZE
is currently defined as:

#define SH_MAX_SIZE (((uint64) PG_UINT32_MAX) + 1)

Can you show a reproducer example where you are able to overflow?

--
Daniel Gustafsson

Avoid overflow with simplehash

[Ranier Vilela]

Hi,

SimpleHash.

The function SH_START_ITERATE can trigger some overflow.

See:
typedef struct SH_ITERATOR
{
uint32 cur; /* current element */
uint32 end;
bool done; /* iterator exhausted? */
} SH_ITERATOR;

The cur field is uint32 size and currently can be stored a uint64,
which obviously does not fit.

Also, the current index is int, which is possibly insufficient
since items can be up to uint32.

Attached a fix.

best regards,
Ranier Vilela


avoid-overflow-with-simplehash-start-iterate.patch
Description: Binary data

Re: Parallel CREATE INDEX for BRIN indexes

[Tomas Vondra]

On 7/5/23 16:33, Matthias van de Meent wrote:
> ...
>
>> Maybe. I wasn't that familiar with what parallel tuplesort can and can't
>> do, and the little I knew I managed to forget since I wrote this patch.
>> Which similar features do you have in mind?
> 
> I was referring to the feature that is "emitting a single sorted run
> of tuples at the leader backend based on data gathered in parallel
> worker backends". It manages the sort state, on-disk runs etc. so that
> you don't have to manage that yourself.
> 
> Adding a new storage format for what is effectively a logical tape
> (logtape.{c,h}) and manually merging it seems like a lot of changes if
> that functionality is readily available, standardized and optimized in
> sortsupport; and adds an additional place to manually go through for
> disk-related changes like TDE.
> 

Here's a new version of the patch, with three main changes:

1) Adoption of the parallel scan approach, instead of the homegrown
solution with a sequence of TID scans. This is mostly what the 0002
patch did, except for fixing a bug - parallel scan has a "rampdown"
close to the end, and this needs to consider the chunk size too.


2) Switches to the parallel tuplesort, as proposed. This turned out to
be easier than I expected - most of the work was in adding methods to
tuplesortvariants.c to allow reading/writing BrinTuple items. The main
limitation is that we need to pass around the length of the tuple
(AFAICS it's not in the BrinTuple itself). I'm not entirely sure about
the memory management aspect of this, and maybe there's a more elegant
solution.

Overall it seems to work - the brin.c code is heavily based on how
nbtsearch.c does parallel builds for btree, so hopefully it's fine. At
some point I got a bit confused about which spool to create/use, but it
seems to work.


3) Handling of empty ranges - I ended up ignoring empty ranges in
workers (i.e. those are not written to the tuplesort), and instead the
leader fills them in when reading data from the shared tuplesort.


One thing I was wondering about is whether it might be better to allow
the workers to process overlapping ranges, and then let the leader to
merge the summaries. That would mean we might not need the tableam.c
changes at all, but the leader would need to do more work (although the
BRIN indexes tend to be fairly small). The main reason that got me
thinking about this is that we have pretty much no tests for the union
procedures, because triggering that is really difficult. But for
parallel index builds that'd be much more common.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 6485c26d0546ec35d81bad33f3893ec5ee421f87 Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Sun, 26 Mar 2023 00:44:01 +0100
Subject: [PATCH] parallel CREATE INDEX for BRIN

---
 src/backend/access/brin/brin.c | 781 -
 src/backend/access/nbtree/nbtsort.c|   2 +-
 src/backend/access/table/tableam.c |  49 +-
 src/backend/access/transam/parallel.c  |   4 +
 src/backend/catalog/index.c|   3 +-
 src/backend/executor/nodeSeqscan.c |   3 +-
 src/backend/utils/sort/tuplesort.c |   3 +
 src/backend/utils/sort/tuplesortvariants.c | 211 ++
 src/include/access/brin.h  |   3 +
 src/include/access/relscan.h   |   1 +
 src/include/access/tableam.h   |   9 +-
 src/include/utils/tuplesort.h  |  11 +
 12 files changed, 1065 insertions(+), 15 deletions(-)

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 3c6a956eaa..9e72c54572 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -33,6 +33,7 @@
 #include "postmaster/autovacuum.h"
 #include "storage/bufmgr.h"
 #include "storage/freespace.h"
+#include "tcop/tcopprot.h"		/* pgrminclude ignore */
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/datum.h"
@@ -40,7 +41,118 @@
 #include "utils/index_selfuncs.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
+#include "utils/tuplesort.h"
 
+/* Magic numbers for parallel state sharing */
+#define PARALLEL_KEY_BRIN_SHARED		UINT64CONST(0xA001)
+#define PARALLEL_KEY_TUPLESORT			UINT64CONST(0xA002)
+#define PARALLEL_KEY_QUERY_TEXT			UINT64CONST(0xA003)
+#define PARALLEL_KEY_WAL_USAGE			UINT64CONST(0xA004)
+#define PARALLEL_KEY_BUFFER_USAGE		UINT64CONST(0xA005)
+
+/*
+ * Status record for spooling/sorting phase.
+ */
+typedef struct BrinSpool
+{
+	Tuplesortstate *sortstate;	/* state data for tuplesort.c */
+	Relation	heap;
+	Relation	index;
+} BrinSpool;
+
+/*
+ * Status for index builds performed in parallel.  This is allocated in a
+ * dynamic shared memory segment.
+ */
+typedef struct BrinShared
+{
+	/*
+	 * These fields are not modified during the build.  They primarily exist for
+	 * the benefit of worker processes that need 

Re: UUID v7

[Tom Lane]

Daniel Gustafsson  writes:
> On 6 Jul 2023, at 15:29, Matthias van de Meent  
> wrote:
>> Sure, it's earlier than the actual release of
>> the standard, but that wasn't a blocker for SQL features that were
>> considered finalized either.

> I can't speak for any SQL standard features we've committed before being
> standardized, it's for sure not the norm for the project.

We have done a couple of things that way recently.  An important
reason why we felt we could get away with that is that nowadays
we have people who actually sit on the SQL committee and have
reliable information on what's likely to make it into the final text
of the next version.  I don't think we have equivalent visibility or
should have equivalent confidence about how UUID v7 standardization
will play out.

regards, tom lane

Re: Problems with estimating OR conditions, IS NULL on LEFT JOINs

[Alena Rybakina]

Hi, all!

On 26.06.2023 12:22, Andrey Lepikhov wrote:

On 24/6/2023 17:23, Tomas Vondra wrote:

I really hope what I just wrote makes at least a little bit of sense.

Throw in one more example:

SELECT i AS id INTO l FROM generate_series(1,10) i;
CREATE TABLE r (id int8, v text);
INSERT INTO r (id, v) VALUES (1, 't'), (-1, 'f');
ANALYZE l,r;
EXPLAIN ANALYZE
SELECT * FROM l LEFT OUTER JOIN r ON (r.id = l.id) WHERE r.v IS NULL;

Here you can see the same kind of underestimation:
Hash Left Join  (... rows=500 width=14) (... rows=9 ...)

So the eqjoinsel_unmatch_left() function should be modified for the 
case where nd1


Unfortunately, this patch could not fix the cardinality calculation in 
this request, I'll try to look and figure out what is missing here.


I tried to fix the cardinality score in the query above by changing:

diff --git a/src/backend/utils/adt/selfuncs.c 
b/src/backend/utils/adt/selfuncs.c

index 8e18aa1dd2b..40901836146 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -2604,11 +2604,16 @@ eqjoinsel_inner(Oid opfuncoid, Oid collation,
 * if we're calculating fraction of NULLs or fraction 
of unmatched rows.

 */
    // unmatchfreq = (1.0 - nullfrac1) * (1.0 - nullfrac2);
-   if (nd1 > nd2)
+   if (nd1 != nd2)
    {
-   selec /= nd1;
-   *unmatched_frac = (nd1 - nd2) * 1.0 / nd1;
+   selec /= Max(nd1, nd2);
+   *unmatched_frac = abs(nd1 - nd2) * 1.0 / 
Max(nd1, nd2);

    }
+   /*if (nd1 > nd2)
+   {
+   selec /= nd1;
+   *unmatched_frac = nd1 - nd2 * 1.0 / nd1;
+   }*/
    else
    {
    selec /= nd2;

and it worked:

SELECT i AS id INTO l FROM generate_series(1,10) i;
CREATE TABLE r (id int8, v text);
INSERT INTO r (id, v) VALUES (1, 't'), (-1, 'f');
ANALYZE l,r;
EXPLAIN ANALYZE
SELECT * FROM l LEFT OUTER JOIN r ON (r.id = l.id) WHERE r.v IS NULL;
ERROR:  relation "l" already exists
ERROR:  relation "r" already exists
INSERT 0 2
ANALYZE
  QUERY PLAN
---
 Hash Left Join  (cost=1.09..1944.13 rows=8 width=14) (actual 
time=0.152..84.184 rows=9 loops=1)

   Hash Cond: (l.id = r.id)
   Filter: (r.v IS NULL)
   Rows Removed by Filter: 2
   ->  Seq Scan on l  (cost=0.00..1443.00 rows=10 width=4) (actual 
time=0.040..27.635 rows=10 loops=1)
   ->  Hash  (cost=1.04..1.04 rows=4 width=10) (actual 
time=0.020..0.022 rows=4 loops=1)

 Buckets: 1024  Batches: 1  Memory Usage: 9kB
 ->  Seq Scan on r  (cost=0.00..1.04 rows=4 width=10) (actual 
time=0.009..0.011 rows=4 loops=1)

 Planning Time: 0.954 ms
 Execution Time: 92.309 ms
(10 rows)

It looks too simple and I suspect that I might have missed something 
somewhere, but so far I haven't found any examples of queries where it 
doesn't work.


I didn't see it breaking anything in the examples from my previous 
letter [1].


1. 
https://www.postgresql.org/message-id/7af1464e-2e24-cfb1-b6d4-1544757f8cfa%40yandex.ru



Unfortunately, I can't understand your idea from point 4, please explain it?

The good thing is this helps even for IS NULL checks on non-join-key
columns (where we don't switch to an antijoin), but there's a couple
things that I dislike ...

1) It's not restricted to outer joins or anything like that (this is
mostly just my laziness / interest in one particular query, but also
something the outer-join-aware patch might help with).

2) We probably don't want to pass this kind of information through
sjinfo. That was the simplest thing for an experimental patch, but I
suspect it's not the only piece of information we may need to pass to
the lower levels of estimation code.

3) I kinda doubt we actually want to move this responsibility (to
consider fraction of unmatched rows) to the low-level estimation
routines (e.g. nulltestsel and various others). AFAICS this just
"introduces NULLs" into the relation, so maybe we could "adjust" the
attribute statistics (in examine_variable?) by inflating null_frac and
modifying the other frequencies in MCV/histogram.

4) But I'm not sure we actually want to do that in these low-level
selectivity functions. The outer join essentially produces output with
two subsets - one with matches on the outer side, one without them. But
the side without matches has NULLs in all columns. In a way, we know
exactly how are these columns correlated - if we do the usual estimation
(even with the null_frac adjusted), we just throw this information away.
And when there's a lot of rows without a match, that seems bad.

--
Regards,
Alena Rybakina
Postgres Professional
From 

Re: pg_basebackup check vs Windows file path limits

[Daniel Gustafsson]

> On 5 Jul 2023, at 14:49, Andrew Dunstan  wrote:
> On 2023-07-04 Tu 16:54, Daniel Gustafsson wrote:
>>> On 4 Jul 2023, at 20:19, Andrew Dunstan 
>>>  wrote:
>>> 
>>> But sadly we're kinda back where we started. fairywren is failing on 
>>> REL_16_STABLE. Before the changes the failure occurred because the test 
>>> script was unable to create the file with a path > 255. Now that we have a 
>>> way to create the file the test for pg_basebackup to reject files with 
>>> names > 100 fails, I presume because the server can't actually see the 
>>> file. At this stage I'm thinking the best thing would be to skip the test 
>>> altogether on windows if the path is longer than 255.
>>> 
>> That does sound like a fairly large hammer for a nail small enough that we
>> should be able to fix it, but I don't have any other good ideas off the cuff.
> 
> Not sure it's such a big hammer. Here's a patch.

No objections to the patch, LGTM.

--
Daniel Gustafsson

Re: UUID v7

[Daniel Gustafsson]

> On 6 Jul 2023, at 15:29, Matthias van de Meent  
> wrote:
> 
> On Thu, 6 Jul 2023 at 14:24, Daniel Gustafsson  wrote:
>> 
>>> On 22 Jun 2023, at 20:30, Nikolay Samokhvalov  wrote:
>> 
>>> Some small updates as I see them:
>>> - there is revision 7 now in https://github.com/ietf-wg-uuidrev/rfc4122bis
>>> - noticing that there is no commitfest record, I created one:
>> 
>> I will actually go ahead and close this entry in the current CF, not because 
>> we
>> don't want the feature but because it's unlikely that it will go in now given
>> that standardization is still underway.  Comitting anything right now seems
>> premature, we might as well wait for standardization given that we have lots 
>> of
>> time before the v17 freeze.
> 
> I'd like to note that this draft has recently had its last call
> period, and has been proposed for publishing early last month.

Sure, but this document is in AD Evaluation and there are many stages left in
the IESG process, it may still take a fair bit of time before this is done.

> Sure, it's earlier than the actual release of
> the standard, but that wasn't a blocker for SQL features that were
> considered finalized either.

I can't speak for any SQL standard features we've committed before being
standardized, it's for sure not the norm for the project.  I'm only commenting
on this particular Internet standard which we have plenty of time to commit
before v17 without rushing to beat a standards committee.

Also, if you look you can see that I moved it to the next CF in a vague hope
that standardization will be swift (which is admittedly never is).

--
Daniel Gustafsson

Re: Fix last unitialized memory warning

[Tristan Partin]

On Thu Jul 6, 2023 at 3:21 AM CDT, Peter Eisentraut wrote:
> On 05.07.23 23:06, Tristan Partin wrote:
> > Thanks for following up. My system is Fedora 38. I can confirm this is
> > still happening on master.
> > 
> > $ gcc --version
> > gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
> > Copyright (C) 2023 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions.  There is NO
> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > $ meson setup build --buildtype=release
>
> This buildtype turns on -O3 warnings.  We have usually opted against 
> chasing warnings in -O3 level because there are often some 
> false-positive uninitialized variable warnings with every new compiler.
>
> Note that we have set the default build type to debugoptimized, for that 
> reason.

Good to know, thanks.

Regarding the original patch, do you think it is good to be applied?

-- 
Tristan Partin
Neon (https://neon.tech)

Re: UUID v7

[Matthias van de Meent]

On Thu, 6 Jul 2023 at 14:24, Daniel Gustafsson  wrote:
>
> > On 22 Jun 2023, at 20:30, Nikolay Samokhvalov  wrote:
>
> > Some small updates as I see them:
> > - there is revision 7 now in https://github.com/ietf-wg-uuidrev/rfc4122bis
> > - noticing that there is no commitfest record, I created one:
>
> I will actually go ahead and close this entry in the current CF, not because 
> we
> don't want the feature but because it's unlikely that it will go in now given
> that standardization is still underway.  Comitting anything right now seems
> premature, we might as well wait for standardization given that we have lots 
> of
> time before the v17 freeze.

I'd like to note that this draft has recently had its last call
period, and has been proposed for publishing early last month. I don't
know how long this publishing process usually takes, but it seems like
the WG considers the text final, so unless this would take months I
wouldn't mind keeping this patch around as "waiting for external
process to complete". Sure, it's earlier than the actual release of
the standard, but that wasn't a blocker for SQL features that were
considered finalized either.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

Re: [PATCH] psql: Add tab-complete for optional view parameters

[Daniel Gustafsson]

> On 29 Jan 2023, at 11:19, Mikhail Gribkov  wrote:

> The new status of this patch is: Waiting on Author

This patch has been Waiting on Author since January with the thread being
stale, so I am marking this as Returned with Feedback for now.  Please feel
free to resubmit to a future CF when there is renewed interest in working on
this.

--
Daniel Gustafsson

Commitfest 2023-07 has started

[Daniel Gustafsson]

We are now a few days in on Commitfest 2023-07, so it seems about time to send
the (by now) customary statistics email on how we are doing, and where we
ideally want go.

There are 350 patches registered in this commitfest, with 150 of those having
been moved from the past commitfest.  If it's not the record, then it's at
least in the top-5 of all times.  Currently the breakdown looks like this:

Needs review: 181
Waiting on Author: 48
Ready for Committer: 38
Committed: 64
Moved to next CF: 3
Withdrawn: 5
Returned with Feedback: 11

Looking at the closed statuses, that means we've already closed 23.7% of all
patches.  Now, that is heavily influenced by bug-fixes being registered in this
CF being closed ahead of the CF as part of the v16 cycle, but it's still very
good.  Let's focus on reducing the number of patches in Needs Review in order
to get them closer to being committable!

I will shortly do a patch triage and send to the list to try and solicit work
on patches which has promise, but have gone stale or slowed down for whatever
reason.

The success of PostgreSQL relies on all of us working together.  If you haven't
yet signed up for reviewing a patch, please consider doing so.  If you are
signed up and actively reviewing, thank you very much!

--
Daniel Gustafsson

Re: [PATCH] Add support function for containment operators

[Kim Johan Andersson]

Any thoughts on this change?

Re: UUID v7

[Daniel Gustafsson]

> On 22 Jun 2023, at 20:30, Nikolay Samokhvalov  wrote:

> Some small updates as I see them:
> - there is revision 7 now in https://github.com/ietf-wg-uuidrev/rfc4122bis
> - noticing that there is no commitfest record, I created one:

I will actually go ahead and close this entry in the current CF, not because we
don't want the feature but because it's unlikely that it will go in now given
that standardization is still underway.  Comitting anything right now seems
premature, we might as well wait for standardization given that we have lots of
time before the v17 freeze.

--
Daniel Gustafsson

Re: Detecting use-after-free bugs using gcc's malloc() attribute

[Peter Eisentraut]

On 28.06.23 20:15, Andres Freund wrote:

On 2023-06-28 10:40:22 +0200, Peter Eisentraut wrote:

On 26.06.23 21:54, Andres Freund wrote:

For something like pg_list.h the malloc(free) attribute is a bit awkward to
use, because one a) needs to list ~30 functions that can free a list and b)
the referenced functions need to be declared.


Hmm.  Saying list_concat() "deallocates" a list is mighty confusing because
1) it doesn't, and 2) it might actually allocate a new list.


list_concat() basically behaves like realloc(), except that the "pointer is
still valid" case is much more common.  And the way that's modelled in the
annotations is to say a function frees and allocates.

Note that the free attribute references the first element for list_concat(),
not the second.


Yeah, I think that would be ok.  I was worried about the cases where it 
doesn't actually free the first argument, but in all those cases it 
passes it as a result, so as far as a caller is concerned, it would 
appear as freed and allocated, even if it's really the same.

Re: Permute underscore separated components of columns before fuzzy matching

[Mikhail Gribkov]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, failed
Documentation:tested, failed

Hello Arne,

The goal of supporting words-switching hints sounds interesting and I've tried 
to apply your patch.
The patch was applied smoothly to the latest master and check-world reported no 
problems. Although I had problems after trying to test the new functionality.

I tried to simply mix words in pg_stat_activity.wait_event_type:

postgres=# select wait_type_event from pg_stat_activity ;
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] ERROR:  column "wait_type_event" does not 
exist at character 8
2023-07-06 14:12:35.968 MSK [1480] HINT:  Perhaps you meant to reference the 
column "pg_stat_activity.wait_event_type".
2023-07-06 14:12:35.968 MSK [1480] STATEMENT:  select wait_type_event from 
pg_stat_activity ;
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
ERROR:  column "wait_type_event" does not 

Re: Bytea PL/Perl transform

[Peter Eisentraut]

On 22.06.23 22:56, Greg Sabino Mullane wrote:
* Do all of these transforms need to be their own contrib modules? So 
much duplicated code across contrib/*_plperl already (and *plpython too 
for that matter) ...


The reason the first transform modules were separate extensions is that 
they interfaced between one extension (plpython, plperl) and another 
extension (ltree, hstore), so it wasn't clear where to put them without 
creating an additional dependency for one of them.


If the transform deals with a built-in type, then they should just be 
added to the respective pl extension directly.

HOT readme missing documentation on summarizing index handling

[Matthias van de Meent]

Hi,

With PG16's 19d8e230, we got rid of BRIN's blocking of HOT updates,
but I just realized that we failed to update the README.HOT document
with this new exception for summarizing indexes.

Attached a patch that updates that document, detailing the related rationale.

I'm not sure if such internal documentation is relevant for
backpatching, but I also don't think it woudl hurt to have this
included in the REL_16_STABLE branch.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech/)


v1-0001-Add-documentation-in-README.HOT-for-handling-summ.patch
Description: Binary data

Re: [PATCH] Add GitLab CI to PostgreSQL

[Andrew Dunstan]

On 2023-07-05 We 11:58, Matthias van de Meent wrote:

On Wed, 5 Jul 2023 at 15:22, Andrew Dunstan  wrote:


On 2023-07-04 Tu 19:44, Newhouse, Robin wrote:


Hello!

I propose the attached patch to be applied on the 'master' branch
of PostgreSQL to add GitLab CI automation alongside Cirrus CI in the PostgreSQL 
repository.

Can you configure GitLab to use a .gitlab-ci.yml file that is not in
the root directory?


It is not intended to be a replacement for Cirrus CI, but simply suggestion for 
the
PostgreSQL project to host centrally a Gitlab CI definition for those who 
prefer to use
it while developing/testing PostgreSQL.

The intent is to facilitate collaboration among GitLab users, promote 
standardization
and consistency, and ultimately, improve testing and code quality.


This seems very RedHat-centric, which I'm not sure is a good idea. Also, 
shouldn't at least some of these recipes call dnf and dnf-builddep instead of 
yum and yum-build-dep?

I don't think it's bad to add an automated test suite for redhat-based images?



I didn't suggest it wasn't just that the coverage should be broader.





If we're going to do this then arguably we should also be supporting GitHub 
Actions and who knows what other CI frameworks. There is a case for us special 
casing Cirrus CI because it's used for the cfbot.

I think there's a good case for _not_ using Cirrus CI, namely that the
license may be prohibitive, self-management of the build system
(storage of artifacts, UI, database) is missing for Cirrus CI, and it
also seems to be unable to run automated CI on repositories that
aren't hosted on Github.
I think these are good arguments for adding a GitLab CI configuration.

Unless the cfbot is entirely under management of the PostgreSQL
project (which it doesn't appear to be, as far as I know the URL is
still cfbot.cputube.org indicating some amount of external control)
the only special casing for Cirrus CI within the project seems to be
"people have experience with this tool", which is good, but not
exclusive to Cirrus CI - clearly there are also people here who have
experience with (or are interested in) maintaining GitLab CI
configurations.



I would not assume too much from the URL. The PostgreSQL BuildFarm 
operated for many years with a URL that was not under postgresql.org. I 
assume the URL is partly a function of the fact that Thomas started the 
cfbot as a bit of a skunkworks project. However it's run, the fact is 
that the project relies to some extent on it.



cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: Permute underscore separated components of columns before fuzzy matching

[Mikhail Gribkov]

Hello Arne,

The goal of supporting words-switching hints sounds interesting and I've
tried to apply your patch.
The patch was applied smoothly to the latest master and check-world
reported no problems. Although I had problems after trying to test the new
functionality.

I tried to simply mix words in pg_stat_activity.wait_event_type:

postgres=# select wait_type_event from pg_stat_activity ;
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] ERROR:  column "wait_type_event" does
not exist at character 8
2023-07-06 14:12:35.968 MSK [1480] HINT:  Perhaps you meant to reference
the column "pg_stat_activity.wait_event_type".
2023-07-06 14:12:35.968 MSK [1480] STATEMENT:  select wait_type_event from
pg_stat_activity ;
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
ERROR:  column "wait_type_event" does not exist
LINE 1: select wait_type_event from pg_stat_activity ;
   ^
HINT:  Perhaps you meant to reference the column
"pg_stat_activity.wait_event_type".
postgres=#

So the desired hint is really there, but thgether with looots of warnings.
For sure these 

Re: MERGE ... RETURNING

[jian he]

On Thu, Jul 6, 2023 at 1:13 PM Gurjeet Singh  wrote:
>
> On Sat, Jul 1, 2023 at 4:08 AM Dean Rasheed  wrote:
> >
> > On Mon, 13 Mar 2023 at 13:36, Dean Rasheed  wrote:
> > >
> > > And another rebase.
> > >
> >
> > I ran out of cycles to pursue the MERGE patches in v16, but hopefully
> > I can make more progress in v17.
> >
> > Looking at this one with fresh eyes, it looks mostly in good shape.
>
> +1
>
> Most of the review was done with the v6 of the patch, minus the doc
> build step. The additional changes in v7 of the patch were eyeballed,
> and tested with `make check`.
>
> > To
> > recap, this adds support for the RETURNING clause in MERGE, together
> > with new support functions pg_merge_action() and
> > pg_merge_when_clause() that can be used in the RETURNING list of MERGE
>
> nit: s/can be used in/can be used only in/
>
> > to retrieve the kind of action (INSERT/UPDATE/DELETE), and the index
> > of the WHEN clause executed for each row merged. In addition,
> > RETURNING support allows MERGE to be used as the source query in COPY
> > TO and WITH queries.
> >
> > One minor annoyance is that, due to the way that pg_merge_action() and
> > pg_merge_when_clause() require access to the MergeActionState, they
> > only work if they appear directly in the RETURNING list. They can't,
> > for example, appear in a subquery in the RETURNING list, and I don't
> > see an easy way round that limitation.
>
> I believe that's a serious limitation, and would be a blocker for the feature.
>
> > Attached is an updated patch with some cosmetic updates, plus updated
> > ruleutils support.
>
> With each iteration of your patch, it is becoming increasingly clear
> that this will be a documentation-heavy patch :-)
>
> I think the name of function pg_merge_when_clause() can be improved.
> How about pg_merge_when_clause_ordinal().
>

> I think the name of function pg_merge_when_clause() can be improved.
> How about pg_merge_when_clause_ordinal().

another idea: pg_merge_action_ordinal()

Re: MERGE ... RETURNING

[Alvaro Herrera]

On 2023-Jul-05, Gurjeet Singh wrote:

> +BEGIN;
> +COPY (
> +MERGE INTO sq_target t
> +USING v
> +ON tid = sid
> +WHEN MATCHED AND tid > 2 THEN
> +UPDATE SET balance = t.balance + delta
> +WHEN NOT MATCHED THEN
> +INSERT (balance, tid) VALUES (balance + delta, sid)
> +WHEN MATCHED AND tid < 2 THEN
> +DELETE
> +RETURNING pg_merge_action(), t.*
> +) TO stdout;
> +DELETE  1   100
> +ROLLBACK;
> 
> I expected the .out file to have captured the stdout. I'm gradually,
> and gladly, re-learning bits of the test infrastructure.
> 
> The DELETE command tag in the output does not feel appropriate for a
> COPY command that's using MERGE as the source of the data.

You misread this one :-)  The COPY output is there, the tag is not.  So
DELETE is the value from pg_merge_action(), and "1 100" correspond to
the columns in the the sq_target row that was deleted.  The command tag
is presumably MERGE 1.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: [PATCH] Add loongarch native checksum implementation.

[YANG Xudong]

On 2023/7/6 15:14, huchangqi wrote:

Hi, i have a loongarch machine runing Loongnix-server (based on redhat/centos, 
it has gcc-8.3 on it), i am trying to test buildfarm on it, when i edit 
build-farm.conf it seems to need animal and secret to connect the buildfarm 
server.
then i go to  https://buildfarm.postgresql.org/cgi-bin/register-form.pl, and 
registered the buildfarm  a week ago, but didn't receive any response. so what 
else i need to do next.




Is it possible to provide a build farm instance for new world ABI of 
loongarch also by loongson? It will be really appreciated.


Thanks!




-Original Messages-
From: "YANG Xudong" 
Send time:Wednesday, 07/05/2023 10:15:51
To: "John Naylor" 
Cc: pgsql-hackers@lists.postgresql.org, wengyanq...@ymatrix.cn, 
wang...@ymatrix.cn
Subject: Re: [PATCH] Add loongarch native checksum implementation.

Is there any other comment?

If the patch looks OK, I would like to update its status to ready for
committer in the commitfest.

Thanks!

On 2023/6/16 09:28, YANG Xudong wrote:

Updated the patch based on the comments.

On 2023/6/15 18:30, John Naylor wrote:


On Wed, Jun 14, 2023 at 9:20 AM YANG Xudong mailto:yangxud...@ymatrix.cn>> wrote:
  >
  > Attached a new patch with fixes based on the comment below.

Note: It's helpful to pass "-v" to git format-patch, to have different
versions.



Added v2


  > > For x86 and Arm, if it fails to link without an -march flag, we
allow
  > > for a runtime check. The flags "-march=armv8-a+crc" and
"-msse4.2" are
  > > for instructions not found on all platforms. The patch also
checks both
  > > ways, and each one results in "Use LoongArch CRC instruction
  > > unconditionally". The -march flag here is general, not specific. In
  > > other words, if this only runs inside "+elif host_cpu ==
'loongarch64'",
  > > why do we need both with -march and without?
  > >
  >
  > Removed the elif branch.

Okay, since we've confirmed that no arch flag is necessary, some other
places can be simplified:

--- a/src/port/Makefile
+++ b/src/port/Makefile
@@ -98,6 +98,11 @@ pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_CRC)
   pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_CRC)
   pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_CRC)

+# all versions of pg_crc32c_loongarch.o need CFLAGS_CRC
+pg_crc32c_loongarch.o: CFLAGS+=$(CFLAGS_CRC)
+pg_crc32c_loongarch_shlib.o: CFLAGS+=$(CFLAGS_CRC)
+pg_crc32c_loongarch_srv.o: CFLAGS+=$(CFLAGS_CRC)

This was copy-and-pasted from platforms that use a runtime check, so
should be unnecessary.



Removed these lines.


+# If the intrinsics are supported, sets
pgac_loongarch_crc32c_intrinsics,
+# and CFLAGS_CRC.

+# Check if __builtin_loongarch_crcc_* intrinsics can be used
+# with the default compiler flags.
+# CFLAGS_CRC is set if the extra flag is required.

Same here -- it seems we don't need to set CFLAGS_CRC at all. Can you
confirm?



We don't need to set CFLAGS_CRC as commented. I have updated the
configure script to make it align with the logic in meson build script.


  > > Also, I don't have a Loongarch machine for testing. Could you
show that
  > > the instructions are found in the binary, maybe using objdump and
grep?
  > > Or a performance test?
  > >
  >
  > The output of the objdump command `objdump -dS
  > ../postgres-build/tmp_install/usr/local/pgsql/bin/postgres  | grep
-B 30
  > -A 10 crcc` is attached.

Thanks for confirming.

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





本邮件及其附件含有龙芯中科的商业秘密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制或散发）本邮件及其附件中的信息。如果您错收本邮件，请您立即电话或邮件通知发件人并删除本邮件。
This email and its attachments contain confidential information from Loongson 
Technology , which is intended only for the person or entity whose address is 
listed above. Any use of the information contained herein in any way 
(including, but not limited to, total or partial disclosure, reproduction or 
dissemination) by persons other than the intended recipient(s) is prohibited. 
If you receive this email in error, please notify the sender by phone or email 
immediately and delete it.

Re: POC, WIP: OR-clause support for indexes

[Andrey Lepikhov]

On 6/7/2023 03:06, Alena Rybakina wrote:

I corrected this constant in the patch.

The patch don't apply cleanly: it contains some trailing spaces.

Also, quick glance into the code shows some weak points;
1. transformBoolExprOr should have input type BoolExpr.
2. You can avoid the switch operator at the beginning of the function, 
because you only need one option.

3. Stale comments: RestrictIinfos definitely not exists at this point.
4. I don't know, you really need to copy the expr or not, but it is 
better to do as late, as possible.

5. You assume, that leftop is non-constant and rightop - constant. Why?
6.I doubt about equivalence operator. Someone can invent a custom '=' 
operator with another semantics, than usual. May be better to check 
mergejoinability?
7. I don't know how to confidently identify constant expressions at this 
level. So, I guess, You can only merge here expressions like 
"F(X)=Const", not an 'F(X)=ConstExpression'.


See delta.diff with mentioned changes in attachment.

--
regards,
Andrey Lepikhov
Postgres Professional
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index c9193d826f..26648b0876 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -107,41 +107,22 @@ typedef struct OrClauseGroupEntry
 static int const_transform_or_limit = 500;
 
 static Node *
-transformBoolExprOr(ParseState *pstate, Expr *expr_orig)
+transformBoolExprOr(ParseState *pstate, BoolExpr *expr_orig)
 {
List   *or_list = NIL;
List   *groups_list = NIL;
ListCell   *lc_eargs;
Node   *result;
-   BoolExpr   *expr = (BoolExpr *)copyObject(expr_orig);
-   const char *opname;
+   BoolExpr   *expr;
boolchange_apply = false;
-   boolor_statement = false;
-
-   Assert(IsA(expr, BoolExpr));
 
/* If this is not expression "Or", then will do it the old way. */
-   switch (expr->boolop)
-   {
-   case AND_EXPR:
-   opname = "AND";
-   break;
-   case OR_EXPR:
-   opname = "OR";
-   or_statement = true;
-   break;
-   case NOT_EXPR:
-   opname = "NOT";
-   break;
-   default:
-   elog(ERROR, "unrecognized boolop: %d", (int) 
expr->boolop);
-   opname = NULL;  /* keep compiler quiet */
-   break;
-   }
-
-   if (!or_statement || list_length(expr->args) < const_transform_or_limit)
+   if (expr_orig->boolop != OR_EXPR ||
+   list_length(expr_orig->args) < const_transform_or_limit)
return transformBoolExpr(pstate, (BoolExpr *)expr_orig);
 
+   expr = (BoolExpr *)copyObject(expr_orig);
+
/*
* NOTE:
* It is an OR-clause. So, rinfo->orclause is a BoolExpr node, 
contains
@@ -176,15 +157,13 @@ transformBoolExprOr(ParseState *pstate, Expr *expr_orig)
if (!arg)
break;
 
-   allow_transformation = (
-   or_statement &&
-   arg->type == T_A_Expr && (arg)->kind == 
AEXPR_OP &&
+   allow_transformation = (arg->type == T_A_Expr && (arg)->kind == 
AEXPR_OP &&

list_length((arg)->name) >=1 && strcmp(strVal(linitial((arg)->name)), "=") == 0
   );
 
 
bare_orarg = transformExprRecurse(pstate, (Node *)arg);
-   bare_orarg = coerce_to_boolean(pstate, bare_orarg, opname);
+   bare_orarg = coerce_to_boolean(pstate, bare_orarg, "OR");
 
/*
 * The next step: transform all the inputs, and detect whether 
any contain
@@ -357,14 +336,10 @@ transformBoolExprOr(ParseState *pstate, Expr *expr_orig)
}
}
 
-   if (!change_apply)
-   {
-   or_statement = false;
-   }
list_free_deep(groups_list);
}
 
-   if (or_statement)
+   if (change_apply)
{
/* One more trick: assemble correct clause */
expr = list_length(or_list) > 1 ? makeBoolExpr(OR_EXPR, 
list_copy(or_list), -1) : linitial(or_list);
@@ -376,9 +351,8 @@ transformBoolExprOr(ParseState *pstate, Expr *expr_orig)
 * transformBoolExpr function
 */
else
-   {
result = transformBoolExpr(pstate, (BoolExpr *)expr_orig);
-   }
+
list_free(or_list);
 
return result;
@@ -496,7 +470,7 @@ transformExprRecurse(ParseState *pstate, Node *expr)
  

Re: MarkGUCPrefixReserved() doesn't check all options

[Heikki Linnakangas]

On 06/07/2023 12:17, Karina Litskevich wrote:

Hi hackers,

Ekaterina Sokolova and I have found a bug in PG 15. Since 88103567cb 
MarkGUCPrefixReserved() is supposed not only reporting GUCs that

haven't been defined by the extension, but also removing them.
However, it removes them in a wrong way, so that a GUC that goes
after the removed GUC is never checked.

To reproduce the bug add the following to the postgresql.conf

shared_preload_libraries = 'pg_stat_statements'
pg_stat_statements.nonexisting_option_1 = on
pg_stat_statements.nonexisting_option_2 = on
pg_stat_statements.nonexisting_option_3 = on
pg_stat_statements.nonexisting_option_4 = on

and start the server. In the logfile you'll see only first and third
options reported invalid and removed.


Good catch!


In master MarkGUCPrefixReserved() iterates a hash table, not an array
as in PG 15. I'm not sure whether it is safe to remove an entry from
this hash table while iterating it, but at least I can't reproduce
the bug on master.
Yes, it's safe to remove the current element, while scanning a hash 
table with hash_seq_init/search. See comment on hash_seq_init.



I attached a bugfix for PG 15.


Applied, thanks!

--
Heikki Linnakangas
Neon (https://neon.tech)

Re: Latches vs lwlock contention

[Peter Eisentraut]

On 04.03.23 20:50, Thomas Munro wrote:

Subject: [PATCH v3 1/6] Allow palloc_extended(NO_OOM) in critical sections.

Commit 4a170ee9e0e banned palloc() and similar in critical sections, because an
allocation failure would produce a panic.  Make an exception for allocation
with NULL on failure, for code that has a backup plan.


I suppose this assumes that out of memory is the only possible error 
condition that we are concerned about for this?


For example, we sometimes see "invalid memory alloc request size" either 
because of corrupted data or because code does things we didn't expect. 
This would then possibly panic?  Also, the realloc code paths 
potentially do more work with possibly more error conditions, and/or 
they error out right away because it's not supported by the context type.


Maybe this is all ok, but it would be good to make the assumptions more 
explicit.

Re: Make ON_ERROR_STOP stop on shell script failure

[Daniel Gustafsson]

This patch hasn't applied for quite some time, has been waiting on author since
December, and the thread has stalled.  I'm marking this Returned with Feedback
for now, please feel free to resubmit to a future CF when there is renewed
interest in working on this.

--
Daniel Gustafsson

Re: [PATCH] Add loongarch native checksum implementation.

[Daniel Gustafsson]

> On 6 Jul 2023, at 09:14, huchangqi  wrote:
> 
> Hi, i have a loongarch machine runing Loongnix-server (based on 
> redhat/centos, it has gcc-8.3 on it), i am trying to test buildfarm on it, 
> when i edit build-farm.conf it seems to need animal and secret to connect the 
> buildfarm server.
> then i go to  https://buildfarm.postgresql.org/cgi-bin/register-form.pl, and 
> registered the buildfarm  a week ago, but didn't receive any response. so 
> what else i need to do next.

Thanks for volunteering a buildfarm animal!  The registration is probably just
pending due to it being summer and things slow down, but I've added Andrew
Dunstan who is the Buildfarm expert on CC:.

--
Daniel Gustafsson

Re: EBCDIC sorting as a use case for ICU rules

[Peter Eisentraut]

On 30.06.23 13:08, Daniel Verite wrote:

About making a doc patch from this, I've came up with the attached,
which generates a CREATE COLLATION statement with rules from an
arbitrary strings that just lists characters in whichever order is desired.


I like adding more documentation and links around this.  But I'm not 
sure how this code you are including is supposed to help users 
understand the rules language.  Effectively, this would be adding 
another rules mechanism on top of the existing one, but doesn't explain 
either one.

Re: EBCDIC sorting as a use case for ICU rules

[Peter Eisentraut]

On 21.06.23 15:28, Daniel Verite wrote:

A collation like the following this seems to work (the rule simply enumerates
US-ASCII letters in the EBCDIC alphabet order, with adequate quoting)

CREATE COLLATION ebcdic (provider='icu', locale='und',
rules=$$&'
'<'.'<'<'<'('<'+'<\|<'&'<'!'<'$'<'*'<')'<';'<'-'<'/'<','<'%'<'_'<'>'<'?'<'`'<':'<'#'<'@'<\'<'='<'"'?`:#@'="abcdefghijklmnopqr~stuvwxyz[^]{ABCDEFGHI}JKLMNOPQR\ST
UVWXYZ0123456789

Maybe this example could be added to the documentation except for
the problem that the rule is very long and dollar-quoting cannot be split
into several lines. Literals enclosed by single quotes can be split that
way, but would require escaping the single quotes in the rule, which
would lead to scary-looking over-quoted contents.


You can use whitespace in the rules.  For example,

CREATE COLLATION ebcdic (provider='icu', locale='und',
rules=$$
& ' ' < '.' < '<' < '(' < '+' < \|
< '&' < '!' < '$' < '*' < ')' < ';'
< '-' < '/' < ',' < '%' < '_' < '>' < '?'
< '`' < ':' < '#' < '@' < \' < '=' < '"'
< a < b < c < d < e < f < g < h < i
< j < k < l < m < n < o < p < q < r
< '~' < s < t < u < v < w < x < y < z
< '[' < '^' < ']'
< '{' < A < B < C < D < E < F < G < H < I
< '}' < J < K < L < M < N < O < P < Q < R
< '\'  < S < T < U < V < W < X < Y < Z
< 0 < 1 < 2 < 3 < 4 < 5 < 6 < 7 < 8 < 9
$$);

(This particular layout is meant to match the rows in
https://en.wikipedia.org/wiki/EBCDIC#Code_page_layout.)

Re: logicalrep_message_type throws an error

[Amit Kapila]

On Wed, Jul 5, 2023 at 7:54 PM Ashutosh Bapat
 wrote:
>
> On Wed, Jul 5, 2023 at 7:07 PM Euler Taveira  wrote:
> >
> > On Wed, Jul 5, 2023, at 7:56 AM, Alvaro Herrera wrote:
> >
> > On 2023-Jul-05, Amit Kapila wrote:
> >
> > > I think after returning "???" from logicalrep_message_type(), the
> > > above is possible when we get the error: "invalid logical replication
> > > message type "X"" from apply_dispatch(), right? If so, then what about
> > > the case when we forgot to handle some message in
> > > logicalrep_message_type() but handled it in apply_dispatch()?
>
> apply_dispatch() has a default case in switch() so it can
> theoretically forget to handle some message type. I think we should
> avoid default case in that function to catch missing message type in
> that function. But if logicalrep_message_type() doesn't use default
> case, it won't forget to handle a known message type. So what you are
> suggesting is not possible.
>

Right, but still I feel it would be better to return actual action.

> It might happen that the upstream may send an unknown message type
> that both apply_dispatch() and logicalrep_message_type() can not
> handle.
>
> > ERROR:  invalid logical replication message type "X"
> > CONTEXT:  processing remote data for replication origin "pg_16638" during 
> > message type "??? (88)" in transaction 796, finished at 0/1626698
> >
> > IMO it could be confusing if we provide two representations of the same 
> > data (X
> > and 88). Since we already provide "X" I don't think we need "88". Another
> > option, is to remove "X" from apply_dispatch() and add "??? (88)" to
> > logicalrep_message_type().
>
> I think we don't need message type to be mentioned in the context for
> an error about invalid message type. I think what needs to be done is
> to set
> apply_error_callback_arg.command = 0 before calling ereport in the
> default case of apply_dispatch().
> apply_error_callback() will just return without providing a context.
>

Hmm, this looks a bit hacky to me.

-- 
With Regards,
Amit Kapila.

Re: logicalrep_message_type throws an error

[Amit Kapila]

On Wed, Jul 5, 2023 at 10:45 PM Alvaro Herrera  wrote:
>
> On 2023-Jul-05, Alvaro Herrera wrote:
>
> > On 2023-Jul-05, Euler Taveira wrote:
> >
> > > Isn't this numerical value already exposed in the error message (X = 88)?
> > > In this example, it is:
> > >
> > > ERROR:  invalid logical replication message type "X"
> > > CONTEXT:  processing remote data for replication origin "pg_16638" during 
> > > message type "??? (88)" in transaction 796, finished at 0/1626698
> > >
> > > IMO it could be confusing if we provide two representations of the same 
> > > data (X
> > > and 88). Since we already provide "X" I don't think we need "88".
> >
> > The CONTEXT message could theoretically appear in other error throws,
> > not just in "invalid logical replication message".  So the duplicity is
> > not really a problem.
>
> Ah, but you're thinking that if the message type is invalid, then it
> will have been rejected in the "invalid logical replication message"
> stage, so no invalid message type will be reported.
>

Yeah, but it would still be displayed both in context and the actual message.

>  I guess there's a
> point to that argument as well.
>

One point to note is that the user may also get confused if the actual
ERROR says message type as 'X' and the context says '???'. I feel in
this case duplicate information is better than different information.

-- 
With Regards,
Amit Kapila.

MarkGUCPrefixReserved() doesn't check all options

[Karina Litskevich]

Hi hackers,

Ekaterina Sokolova and I have found a bug in PG 15. Since 88103567cb
MarkGUCPrefixReserved() is supposed not only reporting GUCs that haven't
been
defined by the extension, but also removing them. However, it removes them
in
a wrong way, so that a GUC that goes after the removed GUC is never checked.

To reproduce the bug add the following to the postgresql.conf

shared_preload_libraries = 'pg_stat_statements'
pg_stat_statements.nonexisting_option_1 = on
pg_stat_statements.nonexisting_option_2 = on
pg_stat_statements.nonexisting_option_3 = on
pg_stat_statements.nonexisting_option_4 = on

and start the server. In the logfile you'll see only first and third options
reported invalid and removed.

In master MarkGUCPrefixReserved() iterates a hash table, not an array as in
PG 15. I'm not sure whether it is safe to remove an entry from this hash
table
while iterating it, but at least I can't reproduce the bug on master.

I attached a bugfix for PG 15.

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From 5f0e59b225ebb408a0c971bc78e22050b296ae9a Mon Sep 17 00:00:00 2001
From: Karina Litskevich 
Date: Thu, 6 Jul 2023 11:00:36 +0300
Subject: [PATCH v1] Fix MarkGUCPrefixReserved() to check all options

---
 src/backend/utils/misc/guc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 915f557c68..c410ba532d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9723,6 +9723,7 @@ MarkGUCPrefixReserved(const char *className)
 			num_guc_variables--;
 			memmove(_variables[i], _variables[i + 1],
 	(num_guc_variables - i) * sizeof(struct config_generic *));
+			i--;
 		}
 	}
 
-- 
2.25.1

Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes

[Amit Kapila]

On Wed, Jul 5, 2023 at 7:20 PM Ashutosh Bapat
 wrote:
>
> On Wed, Jul 5, 2023 at 2:29 PM Amit Kapila  wrote:
> >
> > On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila  wrote:
> > >
> > > On Mon, Jul 3, 2023 at 4:49 PM vignesh C  wrote:
> > > >
> > > > +1 for the first version patch, I also felt the first version is
> > > > easily understandable.
> > > >
> > >
> > > Okay, please find the slightly updated version (changed a comment and
> > > commit message). Unless there are more comments, I'll push this in a
> > > day or two.
> > >
> >
> > oops, forgot to attach the patch.
>
> I still think that we need to do something so that a new call to
> pg_output_begin() automatically takes care of the conditions under
> which it should be called. Otherwise, we will introduce a similar
> problem in some other place in future.
>

AFAIU, this problem is because we forget to conditionally call
pg_output_begin() from pg_decode_message() which can happen with or
without moving conditions inside pg_output_begin(). Also, it shouldn't
be done at the expense of complexity. I find the check added by
Vignesh's v2 patch (+ if (!(last_write ^ data->skip_empty_xacts) ||
txndata->xact_wrote_changes)) a bit difficult to understand and more
error-prone. The others like Hou-San also couldn't understand unless
Vignesh gave an explanation. I also thought of avoiding that check.
Basically, IIUC, the check is added because the patch also removed
'data->skip_empty_xacts' check from
pg_decode_begin_txn()/pg_decode_begin_prepare_txn(). Now, if retain
those checks then it is probably okay but again the similar checks are
still split and that doesn't appear to be better than the v1 or v3
patch version. I am not against improving code in this area and
probably we can consider doing it as a separate patch if we have
better ideas instead of combining it with this patch.

-- 
With Regards,
Amit Kapila.

Re: Fix order of checking ICU options in initdb and create database

[Daniel Gustafsson]

This patch has now been waiting for author since December, with the thread
stalled.  I am marking this returned with feedback for now, please feel free to
re-submit the patch to a future CF when there is renewed interest in working on
this.

--
Daniel Gustafsson

Re: Add estimated hit ratio to Memoize in EXPLAIN to explain cost adjustment

[Lukas Fittl]

On Thu, Jul 6, 2023 at 12:56 AM Daniel Gustafsson  wrote:

> Lukas: do you have an updated patch for this commitfest to address David's
> comments?
>

I have a draft - I should be able to post an updated patch in the next
days. Thanks for checking!

Thanks,
Lukas

-- 
Lukas Fittl

Re: SQL:2011 application time

[Daniel Gustafsson]

> On 6 Jul 2023, at 10:12, Peter Eisentraut  
> wrote:

> it would make sense to either close this entry as returned with feedback, or 
> move it to the next commit fest as waiting on author.

Fair enough, done.

--
Daniel Gustafsson

Re: Fix last unitialized memory warning

[Peter Eisentraut]

On 05.07.23 23:06, Tristan Partin wrote:

Thanks for following up. My system is Fedora 38. I can confirm this is
still happening on master.

$ gcc --version
gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ meson setup build --buildtype=release


This buildtype turns on -O3 warnings.  We have usually opted against 
chasing warnings in -O3 level because there are often some 
false-positive uninitialized variable warnings with every new compiler.


Note that we have set the default build type to debugoptimized, for that 
reason.

Re: [PATCH] ltree hash functions

[Daniel Gustafsson]

> On 19 Jun 2023, at 11:18, Tommy Pavlicek  wrote:

> Tommy, are you interested in extending ALTER OPERATOR to allow this,
> which would also allow fixing the ltree operator?
> 
> Yes, I can do that. I took a look over the code and email thread and it seems 
> like it should be relatively straight forward. I'll put a patch together for 
> that and then update this patch to alter the operator.

Did you have a chance to look at this for an updated patch for this commitfest?

--
Daniel Gustafsson

Re: SQL:2011 application time

[Peter Eisentraut]

On 04.07.23 14:48, Daniel Gustafsson wrote:

On 8 May 2023, at 09:10, Peter Eisentraut  
wrote:

On 03.05.23 23:02, Paul Jungwirth wrote:

Thank you again for the review. Here is a patch with most of your feedback 
addressed. Sorry it has taken so long! These patches are rebased up to 
1ab763fc22adc88e5d779817e7b42b25a9dd7c9e
(May 3).


Here are a few small fixup patches to get your patch set compiling cleanly.

Also, it looks like the patches 0002, 0003, and 0004 are not split up 
correctly.  0002 contains tests using the FOR PORTION OF syntax introduced in 
0003, and 0003 uses the function build_period_range() from 0004.


These patches no longer apply without a new rebase.  Should this patch be
closed in while waiting for the prequisite of adding btree_gist to core
mentioned upthread?  I see no patch registered in the CF for this unless I'm
missing sometihng.


I had talked to Paul about this offline a while ago.  btree_gist to core 
is no longer considered a prerequisite.  But Paul was planning to 
produce a new patch set that is arranged and sequenced a bit 
differently.  Apparently, that new version is not done yet, so it would 
make sense to either close this entry as returned with feedback, or move 
it to the next commit fest as waiting on author.

Re: Exclusion constraints on partitioned tables

[Peter Eisentraut]

On 17.03.23 17:03, Paul Jungwirth wrote:
Thank you for taking a look! I did some research on the history of the 
code here, and I think I understand Tom's concern about making sure the 
index uses the same equality operator as the partition. I was confused 
about his remarks about the opfamily, but I agree with you that if the 
operator is the same, we should be okay.


I added the code about RTEqualStrategyNumber because that's what we need 
to find an equals operator when the index is GiST (except if it's using 
an opclass from btree_gist; then it needs to be BTEqual again). But then 
I realized that for exclusion constraints we have already figured out 
the operator (in RelationGetExclusionInfo) and put it in 
indexInfo->ii_ExclusionOps. So we can just compare against that. This 
works whether your index uses btree_gist or not.


Here is an updated patch with that change (also rebased).

I also included a more specific error message. If we find a matching 
column in the index but with the wrong operator, we should say so, and 
not say there is no matching column.


This looks all pretty good to me.  A few more comments:

It seems to me that many of the test cases added in indexing.sql are 
redundant with create_table.sql/alter_table.sql (or vice versa).  Is 
there a reason for this?



This is not really a problem in your patch, but I think in

-   if (partitioned && (stmt->unique || stmt->primary))
+   if (partitioned && (stmt->unique || stmt->primary || 
stmt->excludeOpNames != NIL))


the stmt->primary is redundant and should be removed.  Right now 
"primary" is always a subset of "unique", but presumably a future patch 
of yours wants to change that.



Furthermore, I think it would be more elegant in your patch if you wrote 
stmt->excludeOpNames without the "== NIL" or "!= NIL", so that it 
becomes a peer of stmt->unique.  (I understand some people don't like 
that style.  But it is already used in that file.)



I would consider rearranging some of the conditionals more as a 
selection of cases, like "is it a unique constraint?", "else, is it an 
exclusion constraint?" -- rather than the current "is it an exclusion 
constraint?, "else, various old code".  For example, instead of


if (stmt->excludeOpNames != NIL)
idx_eqop = indexInfo->ii_ExclusionOps[j];
else
idx_eqop = get_opfamily_member(..., eq_strategy);

consider

if (stmt->unique)
idx_eqop = get_opfamily_member(..., eq_strategy);
else if (stmt->excludeOpNames)
idx_eqop = indexInfo->ii_ExclusionOps[j];
Assert(idx_eqop);

Also, I would push the code

if (accessMethodId == BTREE_AM_OID)
eq_strategy = BTEqualStrategyNumber;

further down into the loop, so that you don't have to remember in which 
cases eq_strategy is assigned or not.


(It's also confusing that the eq_strategy variable is used for two 
different things in this function, and that would clean that up.)



Finally, this code

+   att = TupleDescAttr(RelationGetDescr(rel),
+   key->partattrs[i] - 1);
+   ereport(ERROR,
+   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot match partition key 
to index on column \"%s\" using non-equal operator \"%s\".",
+   NameStr(att->attname), 
get_opname(indexInfo->ii_ExclusionOps[j];


could be simplified by using get_attname().


This is all just a bit of polishing.  I think it would be good to go 
after that.

Re: Add estimated hit ratio to Memoize in EXPLAIN to explain cost adjustment

[Daniel Gustafsson]

> On 7 Mar 2023, at 10:51, David Rowley  wrote:
> 
> On Sun, 5 Mar 2023 at 13:21, Lukas Fittl  wrote:
>> Alternatively (or in addition) we could consider showing the "ndistinct" 
>> value that is calculated in cost_memoize_rescan - since that's the most 
>> significant contributor to the cache hit ratio (and you can influence that 
>> directly by improving the ndistinct statistics).
> 
> I think the ndistinct estimate plus the est_entries together would be
> useful. I think showing just the hit ratio number might often just
> raise too many questions about how that's calculated. To calculate the
> hit ratio we need to estimate the number of entries that can be kept
> in the cache at once and also the number of input rows and the number
> of distinct values.  We can see the input rows by looking at the outer
> side of the join in EXPLAIN, but we've no idea about the ndistinct or
> how many items the planner thought could be kept in the cache at once.
> 
> The plan node already has est_entries, so it should just be a matter
> of storing the ndistinct estimate in the Path and putting it into the
> Plan node so the executor has access to it during EXPLAIN.

Lukas: do you have an updated patch for this commitfest to address David's
comments?

--
Daniel Gustafsson

Re: pgsql: Fix search_path to a safe value during maintenance operations.

[Jeff Davis]

On Thu, 2023-06-29 at 22:09 -0700, Nathan Bossart wrote:
> On Thu, Jun 29, 2023 at 08:53:56PM -0400, Tom Lane wrote:
> > I'm leaning to Robert's thought that we need to revert this for
> > now,
> > and think harder about how to make it work cleanly and safely.
> 
> Since it sounds like this is headed towards a revert, here's a patch
> for
> removing MAINTAIN and pg_maintain.

It was difficult to review standalone, so I tried a quick version
myself and ended up with very similar results. The only substantial
difference was that I put back:


+   if (!vacuum_is_relation_owner(relid, classForm,
options))
+   continue;


in get_all_vacuum_rels() whereas your patch left it out -- double-check
that we're doing the right thing there.

Also remember to bump the catversion. Other than that, it looks good to
me.

Regards,
Jeff Davis

Re: RFC: pg_stat_logmsg

[Masahiko Sawada]

Hi,

On Sat, Jul 1, 2023 at 8:57 AM Joe Conway  wrote:
>
> Greetings,
>
> Attached please find a tarball (rather than a patch) for a proposed new
> contrib extension, pg_stat_logmsg.
>
> The basic idea is to mirror how pg_stat_statements works, except the
> logged messages keyed by filename, lineno, and elevel are saved with a
> aggregate count. The format string is displayed (similar to a query
> jumble) for context, along with function name and sqlerrcode.
>
>
> Part of the thinking is that people with fleets of postgres instances
> can use this to scan for various errors that they care about.
> Additionally it would be useful to look for "should not happen" errors.

Interesting idea and use cases.

I'm concerned that displaying the format string could not be helpful
in some cases. For example, when raising an ERROR while reading WAL
records, we typically write the error message stored in
record->errormsg_buf:

in ReadRecord():
if (errormsg)
ereport(emode_for_corrupt_record(emode, xlogreader->EndRecPtr),
(errmsg_internal("%s", errormsg) /* already
translated */ ));

In this case, the error message stored in pg_stat_logmsg is just '%s'.
The filename and line number columns might help identify the actual
error but it requires users to read the source code and cannot know
the actual error message.

A similar case is where we construct the error message on the fly. For
example, in LogRecoveryConflict() the string of the recovery conflict
description comes from get_recovery_conflict_desc():

in LogRecoveryConflict():
ereport(LOG,
errmsg("recovery still waiting after %ld.%03d ms: %s",
   msecs, usecs, get_recovery_conflict_desc(reason)),
nprocs > 0 ? errdetail_log_plural("Conflicting process: %s.",
  "Conflicting processes: %s.",
  nprocs, buf.data) : 0);

The user might want to search the error message by the actual conflict
reason, but cannot. In this case, I'd like to see the actual error
message (I'd like to normalize the number part, though).

That being said, using the format string for the error messages like
"ERROR:  relation "nonexist_table" does not exist" makes sense to me
since we can avoid having too many similar entries.

So I feel that we might need to figure out what part of the log
message should be normalized like pg_stat_statement does for query
strings.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Re: [PATCH] Add loongarch native checksum implementation.

[huchangqi]

Hi, i have a loongarch machine runing Loongnix-server (based on redhat/centos, 
it has gcc-8.3 on it), i am trying to test buildfarm on it, when i edit 
build-farm.conf it seems to need animal and secret to connect the buildfarm 
server.
then i go to  https://buildfarm.postgresql.org/cgi-bin/register-form.pl, and 
registered the buildfarm  a week ago, but didn't receive any response. so what 
else i need to do next.



> -Original Messages-
> From: "YANG Xudong" 
> Send time:Wednesday, 07/05/2023 10:15:51
> To: "John Naylor" 
> Cc: pgsql-hackers@lists.postgresql.org, wengyanq...@ymatrix.cn, 
> wang...@ymatrix.cn
> Subject: Re: [PATCH] Add loongarch native checksum implementation.
> 
> Is there any other comment?
> 
> If the patch looks OK, I would like to update its status to ready for 
> committer in the commitfest.
> 
> Thanks!
> 
> On 2023/6/16 09:28, YANG Xudong wrote:
> > Updated the patch based on the comments.
> > 
> > On 2023/6/15 18:30, John Naylor wrote:
> >>
> >> On Wed, Jun 14, 2023 at 9:20 AM YANG Xudong  >> > wrote:
> >>  >
> >>  > Attached a new patch with fixes based on the comment below.
> >>
> >> Note: It's helpful to pass "-v" to git format-patch, to have different 
> >> versions.
> >>
> > 
> > Added v2
> > 
> >>  > > For x86 and Arm, if it fails to link without an -march flag, we 
> >> allow
> >>  > > for a runtime check. The flags "-march=armv8-a+crc" and 
> >> "-msse4.2" are
> >>  > > for instructions not found on all platforms. The patch also 
> >> checks both
> >>  > > ways, and each one results in "Use LoongArch CRC instruction
> >>  > > unconditionally". The -march flag here is general, not specific. In
> >>  > > other words, if this only runs inside "+elif host_cpu == 
> >> 'loongarch64'",
> >>  > > why do we need both with -march and without?
> >>  > >
> >>  >
> >>  > Removed the elif branch.
> >>
> >> Okay, since we've confirmed that no arch flag is necessary, some other 
> >> places can be simplified:
> >>
> >> --- a/src/port/Makefile
> >> +++ b/src/port/Makefile
> >> @@ -98,6 +98,11 @@ pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_CRC)
> >>   pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_CRC)
> >>   pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_CRC)
> >>
> >> +# all versions of pg_crc32c_loongarch.o need CFLAGS_CRC
> >> +pg_crc32c_loongarch.o: CFLAGS+=$(CFLAGS_CRC)
> >> +pg_crc32c_loongarch_shlib.o: CFLAGS+=$(CFLAGS_CRC)
> >> +pg_crc32c_loongarch_srv.o: CFLAGS+=$(CFLAGS_CRC)
> >>
> >> This was copy-and-pasted from platforms that use a runtime check, so 
> >> should be unnecessary.
> >>
> > 
> > Removed these lines.
> > 
> >> +# If the intrinsics are supported, sets 
> >> pgac_loongarch_crc32c_intrinsics,
> >> +# and CFLAGS_CRC.
> >>
> >> +# Check if __builtin_loongarch_crcc_* intrinsics can be used
> >> +# with the default compiler flags.
> >> +# CFLAGS_CRC is set if the extra flag is required.
> >>
> >> Same here -- it seems we don't need to set CFLAGS_CRC at all. Can you 
> >> confirm?
> >>
> > 
> > We don't need to set CFLAGS_CRC as commented. I have updated the 
> > configure script to make it align with the logic in meson build script.
> > 
> >>  > > Also, I don't have a Loongarch machine for testing. Could you 
> >> show that
> >>  > > the instructions are found in the binary, maybe using objdump and 
> >> grep?
> >>  > > Or a performance test?
> >>  > >
> >>  >
> >>  > The output of the objdump command `objdump -dS
> >>  > ../postgres-build/tmp_install/usr/local/pgsql/bin/postgres  | grep 
> >> -B 30
> >>  > -A 10 crcc` is attached.
> >>
> >> Thanks for confirming.
> >>
> >> -- 
> >> John Naylor
> >> EDB: http://www.enterprisedb.com 
> 


本邮件及其附件含有龙芯中科的商业秘密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制或散发）本邮件及其附件中的信息。如果您错收本邮件，请您立即电话或邮件通知发件人并删除本邮件。
 
This email and its attachments contain confidential information from Loongson 
Technology , which is intended only for the person or entity whose address is 
listed above. Any use of the information contained herein in any way 
(including, but not limited to, total or partial disclosure, reproduction or 
dissemination) by persons other than the intended recipient(s) is prohibited. 
If you receive this email in error, please notify the sender by phone or email 
immediately and delete it.