date:20250411

On Fri, Apr 11, 2025 at 10:40:57AM +1200, David Rowley wrote:
> On Fri, 11 Apr 2025 at 02:51, Nathan Bossart  wrote:
>> This probably isn't v18 material, but this reminds me of my idea to change
>> appendStringInfoString() into a macro for appendBinaryStringInfo() so that
>> the compiler can remove the runtime strlen() calls for string literals [0].
>> In most cases, the benefits are probably negligible, but StringInfo is
>> sometimes used in hot paths.
> 
> That one has come up a few times. The most lengthy discussion I
> remember was in [1]. It didn't come to anything, but I don't think
> there were any objections to it, so maybe we should just do it.
> 
> In the thread I did some measurements of binary size increases.  For
> non-compile-time consts, it does mean putting the strlen() call in the
> calling function, which is a bit of overhead in terms of size.  The
> macro trick I suggested should have fixed that, but I admit the macro
> is a bit ugly. The macro version also still has the overhead of having
> to pass the length of the string when it detects a compile-time const.

Thanks for the additional context.

-- 
nathan

Re: Changing shared_buffers without restart

2025-04-11 Thread Ashutosh Bapat

On Mon, Apr 7, 2025 at 2:13 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> Yes, you're right, plain dynamic Barrier does not ensure all available
> processes will be synchronized. I was aware about the scenario you
> describe, it's mentioned in commentaries for the resize function. I was
> under the impression this should be enough, but after some more thinking
> I'm not so sure anymore. Let me try to structure it as a list of
> possible corner cases that we need to worry about:
>
> * New backend spawned while we're busy resizing shared memory. Those
>   should wait until the resizing is complete and get the new size as well.
>
> * Old backend receives a resize message, but exits before attempting to
>   resize. Those should be excluded from coordination.

Should we detach barrier in on_exit()?

>
> * A backend is blocked and not responding before or after the
>   ProcSignalBarrier message was sent. I'm thinking about a failure
>   situation, when one rogue backend is doing something without checking
>   for interrupts. We need to wait for those to become responsive, and
>   potentially abort shared memory resize after some timeout.

Right.

>
> I think a relatively elegant solution is to extend ProcSignalBarrier
> mechanism to track not only pss_barrierGeneration, as a sign that
> everything was processed, but also something like
> pss_barrierReceivedGeneration, indicating that the message was received
> everywhere but not processed yet. That would be enough to allow
> processes to wait until the resize message was received everywhere, then
> use a global Barrier to wait until all processes are finished.  It's
> somehow similar to your proposal to use two signals, but has less
> implementation overhead.

The way it's implemented in v4 still has the disjoint group problem.
Assume backends p1, p2, p3. All three of them are executing
ProcessProcSignalBarrier(). All three of them updated
pss_barrierReceivedGeneration

/* The message is observed, record that */
pg_atomic_write_u64(&MyProcSignalSlot->pss_barrierReceivedGeneration,
shared_gen);

p1, p2 moved faster and reached following code from ProcessBarrierShmemResize()
if (BarrierAttach(barrier) == SHMEM_RESIZE_REQUESTED)
  WaitForProcSignalBarrierReceived(pg_atomic_read_u64(&ShmemCtrl->Generation));

Since all the processes have received the barrier message, p1, p2 move
ahead and go through all the next phases and finish resizing even
before p3 gets a chance to call ProcessBarrierShmemResize() and attach
itself to Barrier. This could happen because it processed some other
ProcSignalBarrier message. p1 and p2 won't wait for p3 since it has
not attached itself to the barrier. Once p1, p2 finish, p3 will attach
itself to the barrier and resize buffers again - reinitializing the
shared memory, which might has been already modified by p1 or p2. Boom
- there's memory corruption.

Either every process has to make sure that all the other extant
backends have attached themselves to the barrier OR somebody has to
ensure that and signal all the backends to proceed. The implementation
doesn't do either.

>
> * Shared memory address space is now reserved for future usage, making
>   shared memory segments clash (e.g. due to memory allocation)
>   impossible.  There is a new GUC to control how much space to reserve,
>   which is called max_available_memory -- on the assumption that most of
>   the time it would make sense to set its value to the total amount of
>   memory on the machine. I'm open for suggestions regarding the name.

With 0006 applied
+ /* Clean up some reserved space to resize into */
+ if (munmap(m->shmem + m->shmem_size, new_size - m->shmem_size) == -1)
ze, m->shmem)));
... snip ...
+ ptr = mremap(m->shmem, m->shmem_size, new_size, 0);

We unmap the portion of reserved address space where the existing
segment would expand into. As long as we are just expanding this will
work. I am wondering how would this work for shrinking buffers? What
scheme do you have in mind?

>
> * There is one more patch to address hugepages remap. As mentioned in
>   this thread above, Linux kernel has certain limitations when it comes
>   to mremap for segments allocated with huge pages. To work around it's
>   possible to replace mremap with a sequence of unmap and map again,
>   relying on the anon file behind the segment to keep the memory
>   content. I haven't found any downsides of this approach so far, but it
>   makes the anonymous file patch 0007 mandatory.

In 0008
if (munmap(m->shmem, m->shmem_size) < 0)
... snip ...
/* Resize the backing anon file. */
if(ftruncate(m->segment_fd, new_size) == -1)
...
/* Reclaim the space */
ptr = mmap(m->shmem, new_size, PROT_READ | PROT_WRITE,
mmap_flags | MAP_FIXED, m->segment_fd, 0);

How are we preventing something get mapped into the space after
m->shmem + newsize? We will need to add an unallocated but reserved
addressed space map after m->shmem+newsize right?

--
Best Wishes,
Ashutosh Bapat

Re: Correct documentation for protocol version

2025-04-11 Thread Dave Cramer

On Fri, 11 Apr 2025 at 09:39, Fujii Masao 
wrote:

>
>
> On 2025/04/11 18:27, Dave Cramer wrote:
> >
> >
> > On Fri, 11 Apr 2025 at 05:05, Fujii Masao  > wrote:
> >
> >
> >
> > On 2025/04/11 5:17, Dave Cramer wrote:
> >  > No, you are correct.
> >  >
> >  > See new patch
> >
> > Thanks for updating the patch!
> >
> > - Identifies the message as a protocol version negotiation
> > + Identifies the message as a protocol version negotiation.
> > + The server sends this message if the requested protocol is
> > + not equal to the version the server supports or the client
> > + requests protocol options that are not recognized.
> > message.

>
> > You added the sentence starting with "The server sends..."
> > between "negotiation" and "message", but it should be placed
> > after "message", right?
> >
> > Even though the requested version is not equal to the latest
> > version that the server supports, if it's older than
> > the latest one, the message is not sent. So how about
> > wording it like this instead:
> >
> > -
> > Identifies the message as a protocol version negotiation message.
> > The server sends this message when the client requests a newer
> > protocol version than the server supports, or when the client
> > includes protocol options that the server does not recognize.
> > -
> >
> > + The protcol version requested by the client unless it is
> higher than the
> > + latest version we support in which case the latest
> protocol version we support.
> >
> > Maybe rewording this for clarity and using “the server
> > instead of “we” would help. For example:
> >
> > -
> > The latest protocol version supported by the server if the client
> > requests a newer protocol version than the server supports.
> > The protocol version requested by the client, otherwise.
> > -
> >
> >
> > Reworded as suggested
>
> Thanks for updating the patch!
>
>
> While checking the code in older branches, I noticed that the returned
> protocol version is always the latest version supported by the server.
> However, as we discussed, in master, the server may return the version
> requested by the client. The change was introduced in commit 516b87502dc.
> So, probably we'll need to update the documentation differently for
> master and the older branches.
>
>
> The patch adds a new explanation about when the NegotiateProtocolVersion
> message is sent. But a similar explanation already exists in protocol.sgml:
>
>NegotiateProtocolVersion
>
> 
>  The server does not support the minor protocol version requested
>  by the client, but does support an earlier version of the
> protocol;
>  this message indicates the highest supported minor version.  This
>  message will also be sent if the client requested unsupported
> protocol
>  options (i.e., beginning with _pq_.) in the
>  startup packet.
>

Well this isn't quite true since if you request 3.0 and have invalid
options it will return 3.0, which is not the highest supported minor
version.


>
> Given that, I'm now wondering if the new description in the patch
> might be redundant.
>
>
> Also, your original concern was that the phrase "Newest minor protocol
> version"
> is inaccurate since the field contains both major and minor version numbers
> (e.g., 3.2). However, based on other usage in protocol.sgml and source
> comments in related code, "minor version" seems to refer to the full
> version
> like 3.2, i.e., not just the minor part, so we might not need to reword it
> after all.
>

IMO the comments should be changed to reflect reality. If 3.2 is a minor
version what is a major version ?

Dave

Re: Correct documentation for protocol version

2025-04-11 Thread Jelte Fennema-Nio

On Fri, 11 Apr 2025 at 22:57, Dave Cramer  wrote:
> Well this isn't quite true since if you request 3.0 and have invalid options 
> it will return 3.0, which is not the highest supported minor version.

Probably good to update this section too then to be similarly correct
as your already updated section. Maybe also good to clarify further
that the version that the server responds with is the protocol version
that will be used during the following communication.

Re: Changing shared_buffers without restart

2025-04-11 Thread Dmitry Dolgov

> On Fri, Apr 11, 2025 at 08:04:39PM GMT, Ashutosh Bapat wrote:
> On Mon, Apr 7, 2025 at 2:13 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
> >
> > Yes, you're right, plain dynamic Barrier does not ensure all available
> > processes will be synchronized. I was aware about the scenario you
> > describe, it's mentioned in commentaries for the resize function. I was
> > under the impression this should be enough, but after some more thinking
> > I'm not so sure anymore. Let me try to structure it as a list of
> > possible corner cases that we need to worry about:
> >
> > * New backend spawned while we're busy resizing shared memory. Those
> >   should wait until the resizing is complete and get the new size as well.
> >
> > * Old backend receives a resize message, but exits before attempting to
> >   resize. Those should be excluded from coordination.
>
> Should we detach barrier in on_exit()?

Yeah, good point.

> > I think a relatively elegant solution is to extend ProcSignalBarrier
> > mechanism to track not only pss_barrierGeneration, as a sign that
> > everything was processed, but also something like
> > pss_barrierReceivedGeneration, indicating that the message was received
> > everywhere but not processed yet. That would be enough to allow
> > processes to wait until the resize message was received everywhere, then
> > use a global Barrier to wait until all processes are finished.  It's
> > somehow similar to your proposal to use two signals, but has less
> > implementation overhead.
>
> The way it's implemented in v4 still has the disjoint group problem.
> Assume backends p1, p2, p3. All three of them are executing
> ProcessProcSignalBarrier(). All three of them updated
> pss_barrierReceivedGeneration
>
> /* The message is observed, record that */
> pg_atomic_write_u64(&MyProcSignalSlot->pss_barrierReceivedGeneration,
> shared_gen);
>
> p1, p2 moved faster and reached following code from 
> ProcessBarrierShmemResize()
> if (BarrierAttach(barrier) == SHMEM_RESIZE_REQUESTED)
>   
> WaitForProcSignalBarrierReceived(pg_atomic_read_u64(&ShmemCtrl->Generation));
>
> Since all the processes have received the barrier message, p1, p2 move
> ahead and go through all the next phases and finish resizing even
> before p3 gets a chance to call ProcessBarrierShmemResize() and attach
> itself to Barrier. This could happen because it processed some other
> ProcSignalBarrier message. p1 and p2 won't wait for p3 since it has
> not attached itself to the barrier. Once p1, p2 finish, p3 will attach
> itself to the barrier and resize buffers again - reinitializing the
> shared memory, which might has been already modified by p1 or p2. Boom
> - there's memory corruption.

It won't reinitialize anything, since this logic is controlled by the
ShmemCtrl->NSharedBuffers, if it's already updated nothing will be
changed.

About the race condition you mention, there is indeed a window between
receiving the ProcSignalBarrier and attaching to the global Barrier in
resize, but I don't think any process will be able to touch buffer pool
while inside this window. Even if it happens that the remapping itself
was blazing fast that this window was enough to make one process late
(e.g. if it was busy handling some other signal as you mention), as I've
showed above it shouldn't be a problem.

I can experiment with this case though, maybe there is a way to
completely close this window to not thing about even potential
scenarios.

> > * Shared memory address space is now reserved for future usage, making
> >   shared memory segments clash (e.g. due to memory allocation)
> >   impossible.  There is a new GUC to control how much space to reserve,
> >   which is called max_available_memory -- on the assumption that most of
> >   the time it would make sense to set its value to the total amount of
> >   memory on the machine. I'm open for suggestions regarding the name.
>
> With 0006 applied
> + /* Clean up some reserved space to resize into */
> + if (munmap(m->shmem + m->shmem_size, new_size - m->shmem_size) == -1)
> ze, m->shmem)));
> ... snip ...
> + ptr = mremap(m->shmem, m->shmem_size, new_size, 0);
>
> We unmap the portion of reserved address space where the existing
> segment would expand into. As long as we are just expanding this will
> work. I am wondering how would this work for shrinking buffers? What
> scheme do you have in mind?

I didn't like this part originally, and after changes to support hugetlb
I think it's worth it just to replace mremap with munmap/mmap. That way
there will be no such question, e.g. if a segment is getting shrinked
the unmapped area will again become a part of the reserved space.

> > * There is one more patch to address hugepages remap. As mentioned in
> >   this thread above, Linux kernel has certain limitations when it comes
> >   to mremap for segments allocated with huge pages. To work around it's
> >   possible to replace mremap with a sequence of unmap and map again,
> >   relying on the anon fil

Re: stats.sql fails during installcheck on mac

> The code in xlog.c filters out the syncs for WAL_SYNC_METHOD_OPEN and
> WAL_SYNC_METHOD_OPEN_DSYNC, wouldn't it be more consistent to do the
> same in the code and the SQL test, using an IN clause with the two
> values that block the syncs rather than a NOT IN clause with the three
> values that allow the syncs?

I actually originally had it this way, but for some reason
felt it would be better to be explicit about the methods we want to test rather
than not test. I can't think of a very compelling reason to go either way, so v2
LGTM.

>> Hmm, that's a little nasty, because it's not showing up in the
>> buildfarm.  It appears from a little testing that the issue only
>> manifests if you have fsync = on, which we generally don't on
> >buildfarm animals.

> right, "make check" does not encounter this because it runs
> with fsync=off, as I mentioned at the top of the thread.

what do you think of this? I think we should set fsync = on
at least for the part of the test that proceeds the 2 checkpoints and
set if back to off at the end of the tests for fsync stats. It is concerning
the tests for the fsync stats are not being exercised in
the buildfarm.


--
Sami Imseih
Amazon Web Services (AWS)

Re: Correct documentation for protocol version

2025-04-11 Thread Jelte Fennema-Nio

On Fri, 11 Apr 2025 at 21:39, Fujii Masao  wrote:
> While checking the code in older branches, I noticed that the returned
> protocol version is always the latest version supported by the server.
> However, as we discussed, in master, the server may return the version
> requested by the client. The change was introduced in commit 516b87502dc.
> So, probably we'll need to update the documentation differently for
> master and the older branches.

No need for different docs. Given that older branches only support 3.0
protocol, there's no way for a client to request a version earlier
than the "latest version supported by the server".

> The patch adds a new explanation about when the NegotiateProtocolVersion
> message is sent. But a similar explanation already exists in protocol.sgml:

Side-comment: I think our protocol docs are pretty annoyingly spread
across two pages.

> Given that, I'm now wondering if the new description in the patch
> might be redundant.
>
>
> Also, your original concern was that the phrase "Newest minor protocol 
> version"
> is inaccurate since the field contains both major and minor version numbers
> (e.g., 3.2). However, based on other usage in protocol.sgml and source
> comments in related code, "minor version" seems to refer to the full version
> like 3.2, i.e., not just the minor part, so we might not need to reword it
> after all.

I quite like the new wording from Dave so +1 from me. I also think for
protocol docs it's especially important to be very precise and leave
very little room for interpretation.

One thing that we should probably clarify though (which was somewhat
clarified in the previous wording) is that we only send this message
if the client requested a major version that the major version that
the server supports. i.e. we will never send a
NegotiateProtocolVersion message to 3.2 if the client requested 4.0.

Re: Some problems regarding the self-join elimination code

2025-04-11 Thread Andrei Lepikhov


On 4/10/25 14:39, Andrei Lepikhov wrote:

On 4/10/25 13:36, Alexander Korotkov wrote:
On Wed, Apr 9, 2025 at 10:39 AM Andrei Lepikhov  
wrote:

It seems we are coming to the conclusion that join removal optimisation
may do something out of ChangeVarNodes resposibility. Before further
complicating of this function code I would like to know opinion of Tom,
who initially proposed [1] to use this routine. May be better a) return
to more specialised change_relid / sje_walker machinery or b) move
ChangeVarNodes out of rewriteManip and make it multi-purpose routine,
allowing to transform expression that may happen after a Var node 
change?


What about adding a callback to ChangeVarNodes_context that would
called for each RestrictInfo after changing varnodes itself?  SJE
could use a callback that replaces OpExpr with NullTest when needed.
I think it is doable, of course. Just looking forward a little, it may 
need more complication in the future (SJE definitely should be widened 
to partitioned tables) and it may be simpler to have two different 
routines for two different stages of planning. 
To provide some food for thought, here is a draft in attachment which 
addresses both issues: RestrictInfo relid replacement and move 
SJE-specific code out of the ChangeVarNodes routine (callback approach).


--
regards, Andrei LepikhovFrom 6b68703d7b38326393da58e42618e4915a0e4590 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" 
Date: Fri, 11 Apr 2025 14:30:33 +0200
Subject: [PATCH v0] Switch the approach to ChangeVarNodes's extensibility.

---
 src/backend/optimizer/plan/analyzejoins.c | 149 +++---
 src/backend/rewrite/rewriteManip.c|  95 ++
 src/include/rewrite/rewriteManip.h|  14 +-
 3 files changed, 161 insertions(+), 97 deletions(-)

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 6b58567f511..0f0ed1785e6 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -74,6 +74,7 @@ static bool is_innerrel_unique_for(PlannerInfo *root,
    List *restrictlist,
    List **extra_clauses);
 static int	self_join_candidates_cmp(const void *a, const void *b);
+static bool ChangeVarNodes_callback(Node *node, void *arg);
 
 
 /*
@@ -397,7 +398,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
 		{
 			Assert(subst > 0);
 
-			ChangeVarNodes((Node *) sjinf->semi_rhs_exprs, relid, subst, 0);
+			ChangeVarNodesExtended((Node *) sjinf->semi_rhs_exprs, relid, subst,
+	0, ChangeVarNodes_callback);
 		}
 	}
 
@@ -458,7 +460,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
 			   sjinfo->ojrelid, subst);
 			Assert(!bms_is_empty(phv->phrels));
 
-			ChangeVarNodes((Node *) phv->phexpr, relid, subst, 0);
+			ChangeVarNodesExtended((Node *) phv->phexpr, relid, subst, 0,
+	ChangeVarNodes_callback);
 
 			Assert(phv->phnullingrels == NULL); /* no need to adjust */
 		}
@@ -512,7 +515,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
 		}
 
 		if (subst > 0)
-			ChangeVarNodes((Node *) otherrel->lateral_vars, relid, subst, 0);
+			ChangeVarNodesExtended((Node *) otherrel->lateral_vars, relid,
+	subst, 0, ChangeVarNodes_callback);
 	}
 }
 
@@ -746,7 +750,8 @@ remove_rel_from_eclass(EquivalenceClass *ec, SpecialJoinInfo *sjinfo,
 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
 
 		if (sjinfo == NULL)
-			ChangeVarNodes((Node *) rinfo, relid, subst, 0);
+			ChangeVarNodesExtended((Node *) rinfo, relid, subst, 0,
+	ChangeVarNodes_callback);
 		else
 			remove_rel_from_restrictinfo(rinfo, relid, sjinfo->ojrelid);
 	}
@@ -1537,7 +1542,8 @@ update_eclasses(EquivalenceClass *ec, int from, int to)
 		em->em_jdomain->jd_relids = adjust_relid_set(em->em_jdomain->jd_relids, from, to);
 
 		/* We only process inner joins */
-		ChangeVarNodes((Node *) em->em_expr, from, to, 0);
+		ChangeVarNodesExtended((Node *) em->em_expr, from, to, 0,
+ChangeVarNodes_callback);
 
 		foreach_node(EquivalenceMember, other, new_members)
 		{
@@ -1571,7 +1577,8 @@ update_eclasses(EquivalenceClass *ec, int from, int to)
 			continue;
 		}
 
-		ChangeVarNodes((Node *) rinfo, from, to, 0);
+		ChangeVarNodesExtended((Node *) rinfo, from, to, 0,
+ChangeVarNodes_callback);
 
 		/*
 		 * After switching the clause to the remaining relation, check it for
@@ -1666,6 +1673,108 @@ add_non_redundant_clauses(PlannerInfo *root,
 	}
 }
 
+static bool
+ChangeVarNodes_callback(Node *node, void *arg)
+{
+	ChangeVarNodes_context *context = (ChangeVarNodes_context *) arg;
+
+	if (node == NULL)
+		return false;
+
+	if (IsA(node, RangeTblRef))
+	{
+		return false;
+	}
+	else if (IsA(node, RestrictInfo))
+	{
+		RestrictInfo   *rinfo = (RestrictInfo *) node;
+		intrelid = -1;
+		bool			is_req_equal =
+			(rinfo->required_relids == rinfo->clause_relids);
+		bool			is_multiple =
+		(bms_membership(rinfo->clause_relids) == B

Re: Feature Recommendations for Logical Subscriptions

2025-04-11 Thread YeXiu

Amit Kapila, Yes, as you mentioned, but I’d like to add that when using the 
exclusion method for newly added columns, there’s no need to modify the 
publication. This is similar to how fields are automatically synchronized when 
columns are unspecified during initial setup. This is also a key reason why 
this approach is valuable.


YeXiu
1518981...@qq.com







 原始邮件
 
   
发件人：Amit Kapila

Re: Feature Recommendations for Logical Subscriptions

2025-04-11 Thread YeXiu

Another permission-related issue involves scenarios where multiple logical 
replication slots exist. If a replication slot grants full data access 
permissions and user accounts are not explicitly bound to specific slots, there 
could be security risks where accounts might connect to high-privilege 
replication slots, potentially leading to data security vulnerabilities.


YeXiu
1518981...@qq.com







 原始邮件
 
   
发件人：Amit Kapila

Re: Improve a few appendStringInfo calls new to v18

2025-04-11 Thread Tom Lane

Peter Eisentraut  writes:
> Would it be useful to augment appendStringInfo() something like this:

> if (VA_ARGS_NARGS() == 0)
>  return appendStringInfoString(str, fmt);

That would change the behavior in edge cases, for instance
appendStringInfo(str, "foo%%bar").  Maybe we'd never hit those,
but on the whole I'm not in love with the idea.

regards, tom lane

Re: [PoC] Federated Authn/z with OAUTHBEARER

2025-04-11 Thread Wolfgang Walther


Jacob Champion:

On Wed, Apr 9, 2025 at 4:42 PM Jelte Fennema-Nio  wrote:

I think your suggestion of not using any .so files would best there (from w 
user perspective). I'd be quite surprised if a static build still resulted in 
me having to manage shared library files anyway.

Done this way in v5. I had planned to separate the implementations by
a #define, but I ran into issues with Makefile.shlib, so I split the
shared and dynamic versions into separate files. I just now realized
that we do something about this exact problem in src/common, so I'll
see if I can copy its technique for the next go round.


I tried to apply this patch to nixpkgs' libpq build [1]. First, I pinned 
a recent commit from master (one where the v5 patch will apply cleanly 
later) and enabled --with-libcurl [2].


At this stage, without the patch applied, I observe the following:

1. The default, dynamically linked, build succeeds and libpq.so is 
linked to libcurl.so as expected!


2. The statically linked build fails during configure:

  checking for curl_multi_init in -lcurl... no
  configure: error: library 'curl' does not provide curl_multi_init

config.log tells me that it can't link to libcurl, because of undefined 
references, for example:


  undefined reference to `psl_is_cookie_domain_acceptable'
  undefined reference to `nghttp2_session_check_request_allowed'

I assume the many libs listed in Libs.private in libcurl.pc are not 
added automatically for this check?



Next, I applied the v5 patch and:

3. Running the same build as in step 1 above (dynamically linked), I can 
see that libpq.so does have some reference to dlopen / libpq-oauth in it 
- good. But libpq-oauth.so itself is not built. The commands I am using 
to build just the libpq package are essentially like this:


  make submake-libpgport
  make submake-libpq
  make -C src/bin/pg_config install
  make -C src/common install
  make -C src/include install
  make -C src/interfaces/libpq install
  make -C src/port install

I tried adding "make submake-libpq-oauth", but that doesn't exist.

When I do "make -C src/interfaces/libpq-oauth", I get this error:

  make: *** No rule to make target 'oauth-curl.o', needed by 
'libpq-oauth-18.so'.  Stop.


Not sure how to proceed to build libpq-oauth.so.


4. The statically linked build fails with the same configure error as above.


I can only test autoconf right now, not meson - don't have a working 
setup for that, yet.


Best,

Wolfgang

[1]: 
https://github.com/NixOS/nixpkgs/blob/master/pkgs/servers/sql/postgresql/libpq.nix

someone else to do the list of acknowledgments

I would like for someone else to prepare the list of acknowledgments in
the release notes this year.

I have been preparing the list of acknowledgments in the release notes
(example: [0]) since PostgreSQL 10 (launched from discussions at PGCon
2017 [1]). I'm looking to hand this off now, so that I'm not hogging
this job forever.

[0]:
https://www.postgresql.org/docs/17/release-17.html#RELEASE-17-ACKNOWLEDGEMENTS
[1]:
https://wiki.postgresql.org/wiki/PgCon_2017_Developer_Meeting#Release_notes_scope.2C_and_giving_credit

I'm happy to train the next person and hand them my tips and scripts,
or they can of course define their own processes.

So that prospective candidates know what they are getting into, the
(my) process is approximately:

1. collect names from git logs in semi-automated way
2. sort, organize, fix, and normalize names
3. check manually against git log
4. commit
5. fix up based on public feedback
6. keep updated until release

The whole thing might take about 20 to 30 hours wall-clock time.

I have found it not useful to start this too early, since you'll get a
lot of new names during the beta period. I have lately usually
started after the August beta release. (Or you can start early and
keep it updated. Again, it's your process.)

Anyone can do this, you don't need to be a committer or developer (but
you'll need to be able to produce a well-formed documentation patch).
However, I suggest that because there is a fair amount of work to
normalize, fix, and transliterate names, it would help if you've been
around for a while and have some passing familiarity with the names of
the people around here. Also, since this list is often cited for
public credit, some care and attention to detail is needed.

So, there is some time to think about this. Please discuss here if
you're interested or have questions.

(This is presupposing that we still want to do this. If you have
other ideas for a better list or no list, this is also the time to
discuss this.)

Re: Things I don't like about \du's "Attributes" column

On Sun, Feb 9, 2025 at 2:11 AM Pavel Luzanov 
wrote:

>
> I don't understand from new commitfest transition guidance
> what to do with status of commitfest entry in this case.
> May be it needs to be closed. And in a case I will be able to propose
> a new version, I will open a new entry.
>
> The commitfest entry now has Needs Review status and stayed in
> the closed January commitfest.
>
> 0. 
> https://www.postgresql.org/message-id/flat/003e3a66-8fcc-4ca0-9e0e-c0afda1c9424%40eisentraut.org
> 1. https://commitfest.postgresql.org/51/4738/
> 2. 
> https://www.postgresql.org/message-id/5341835b-e7be-44dc-b6e5-400e9e3f3...@postgrespro.ru
> 3. 
> https://www.postgresql.org/message-id/CA%2BTgmoZ_uGDb3N8AKHG6nOc5HZPp5Y_ogFhrRbhoVnPHN%2B4t3g%40mail.gmail.com
>
>
As author it is encouraged that you decide whether Waiting on Author or
Withdrawn is the desired status for this patch if you do not want it, as
presented, to be committed.  If you are content with it being committed
as-is it should be marked Review Needed in an Open Commitfest.  Abandoning
this approach and going for what Robert suggested would suggest withdrawing
this CF entry.

However, I do think we are at something committable, though I'd make one,
maybe two, changes to v8.

Valid until -> Password valid until: the timestamp value already forces a
wide column, adding the word Password to the header to clarify what is
valid simply provides the same context that the create role page provides
when it shows the valid until attribute immediately below the password
attribute.  Leaving "valid until" alone retains the attribute name tieback.

Connection limit -> Con. limit: maybe this gets rejected on translation
grounds but the abbreviation here seems obvious and shaves 7 characters off
the mandatory width for a column that occupies 12 characters more than the
values require.

Even without those changes I as reviewer would concur with the proposal and
try to move this on from bike-shedding to a go/no-go decision (by marking
it Ready to Commit) as to whether this is an improvement over the status
quo.

David J.

Re: [PoC] Federated Authn/z with OAUTHBEARER


On 08.04.25 19:44, Jacob Champion wrote:

Would anybody following along be opposed to a situation where
- dynamiclib builds go through the dlopen() shim
- staticlib builds always rely on statically linked symbols


If this can be implemented in a straightforward way, that would be the 
best way, I think.

remove unnecessary explicit type conversion (to char) for appendStringInfoChar function calls

2025-04-11 Thread Mahendra Singh Thalor

Hi,
In the current master code, 3 places we are using appendStringInfoChar
call with explicit type conversion into char. This is not needed as
all other places, we are using direct character to append.

--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -302,7 +302,7 @@ InteractiveBackend(StringInfo inBuf)
 */

/* Add '\0' to make it look the same as message case. */
-   appendStringInfoChar(inBuf, (char) '\0');
+   appendStringInfoChar(inBuf, '\0');

Here, I am attaching a small patch to fix these 3 type conversions on head.

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com


v01_remove-unnecessary-type-conversion-into-char-for-appendStringInfoChar.patch
Description: Binary data

Re: disallow ALTER VIEW SET DEFAULT when the corresponding base relation column is a generated column

On Friday, April 11, 2025, Tom Lane  wrote:

> jian he  writes:
> > CREATE TABLE gtest1 (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
> > CREATE VIEW gtest1v AS SELECT * FROM gtest1;
> > ALTER VIEW gtest1v ALTER COLUMN b SET DEFAULT 100;
>
> > INSERT INTO gtest1v VALUES (8, DEFAULT) returning *;
> > ERROR:  cannot insert a non-DEFAULT value into column "b"
> > DETAIL:  Column "b" is a generated column.
>
> > we can make
> > ALTER VIEW gtest1v ALTER COLUMN b SET DEFAULT 100;
> > error out,
>
> This is not an improvement over having the error happen at run time.
>
> (1) What if the state of the underlying column changes between the
> ALTER VIEW and the INSERT?  Either you have rejected something
> that could have worked, or in the other direction you're going to get
> the run-time error anyway.
>

I concur.  The view is only loosely coupled to the base relation, via the
rewrite rule which is applied at runtime.  Putting checks in place that
strongly couples the two relations adds a coupling burden that we are
better off avoiding.

David J.

Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote

On Thu, Apr 10, 2025 at 11:58:41PM +0530, Mahendra Singh Thalor wrote:
> As per above discussions, for v18, we will not do any change to server
> side to fix the issue of \n\r in database names. But as a cleanup
> patch, we can give an alert to the user by "pg_upgrade --check". As
> per current code, pg_dump and pg_upgrade will fail with "shell
> command" error but in the attached patch, we will give some extra info
> to the user by "pg_upgrade --check" so that they can fix database
> names before trying to upgrade.
> 
> Here, I am attaching a patch which will give a list of invalid
> database names in "pg_upgrade --check". We can consider this as a
> cleanup patch.

Are you proposing this for v18?  I think this is all v19 material at this
point.  Perhaps we could argue this is a bug fix that should be
back-patched, but IMHO that's a bit of a stretch.  I don't sense a
tremendous amount of urgency, either.

-- 
nathan

Re: Prevent an error on attaching/creating a DSM/DSA from an interrupt handler.

2025-04-11 Thread Rahila Syed

Hi Daniel,

Thank you for your review and code improvements.

Please find below some observations.

1. dsm_unpin_mapping(dsm_segment *seg)
+   if (CurrentResourceOwner &&
IsResourceOwnerReleasing(CurrentResourceOwner))
+   return;

Given that the function can return without setting resowner to a
CurrentResourceOwner which is not NULL, shall we change the function
signature to return true when "unpin" is successful and false when not?
This behavior existed earlier too, but adding the check has made it
explicit.
Although this function is not currently in use, it would be beneficial to
make
the API more verbose.

2.  If value of IsResourceOwnerReleasing changes between
dsm_create_descriptor
and attach_internal, the dsm segment and dsa area will end up with
different resource owners.
Earlier the chances of CurrentResourceOwner changing between the two
functions were zero.
May be something can be done to keep resowner assignments under both these
functions
in sync.

Thank you,
Rahila Syed

Re: n_ins_since_vacuum stats for aborted transactions

I spent some time thinking about this today.

> "The tuple counters below, except where noted, are incremented even if the 
> transaction aborts."

I like this idea, and I think it fits good as a blurb under "27.2.2.
Viewing Statistics"

I suggest a slight re-write however.

+  
+   An aborted transaction will also increment tuple-related counters,
unless otherwise noted.
+  

> So, here are the relevant counters, with their treatment of aborted 
> transaction tuples:
>
> seq_tup_read - says live
> idx_tup_fetch - says live
> n_tup_ins - default notice
> n_tup_upd - default notice
> n_tup_del - default notice
> n_mod_since_analyze - inline reason for non-default
> n_ins_since_vacuum - default notice

All the counters mentioned above will increment the number of rows
modified/accessed even in the case of an aborted transaction, except
for n_mod_since_analyze.

> n_live_tup - says live (is this a counter?)
> n_dead_tup - says dead (is this a counter?)

They are not values that are purely incremental. They are incremented
by insert/update/delete for committed transactions, but are also
updated
by VACUUM or VACUUM FULL. So, these will need some inlined description
of their behavior,

> I'm also thinking to reword n_tup_upd, something like:
>
> Total number of rows updated.  Subsets of these updates are also tracked in 
> n_tup_hot_upd and n_tup_newpage_upd to facilitate performance monitoring.

I think the current explanation is clear enough, I am also not too
thrilled about the "...to facilitate performance monitoring." since
the cumulative stats system
as a whole is known to be used to facilitate perf monitoring.

What do you think of the attached?

--
Sami Imseih
Amazon Web Services (AWS)



--
Sami Imseih
Amazon Web Services (AWS)


v3-0001-Clarify-when-aborted-rows-are-tracked-for-tuple-r.patch
Description: Binary data

Re: Horribly slow pg_upgrade performance with many Large Objects

On Tue, Apr 08, 2025 at 12:22:00PM -0500, Nathan Bossart wrote:
> On Tue, Apr 08, 2025 at 01:07:09PM -0400, Tom Lane wrote:
>> Nathan Bossart  writes:
>>> I do think it's worth considering going back to copying
>>> pg_largobject_metadata's files for upgrades from v16 and newer.
>> 
>> (If we do this) I don't see why we'd need to stop at v16.  I'm
>> envisioning that we'd use COPY, which will be dealing in the
>> text representation of aclitems, and I don't think that's changed
>> in a long time.  The sort of thing that would break it is changes
>> in the set of available/default privilege bits for large objects.
> 
> I was thinking of actually reverting commit 12a53c7 for upgrades from v16,
> which AFAICT is the last release where any relevant storage formats changed
> (aclitem changed in v16).  But if COPY gets us pretty close to that and is
> less likely to be disrupted by future changes, it could be a better
> long-term approach.
> 
>> That is, where the dump currently contains something like
>> 
>> SELECT pg_catalog.lo_create('2121');
>> ALTER LARGE OBJECT 2121 OWNER TO postgres;
>> GRANT ALL ON LARGE OBJECT 2121 TO joe;
>> 
>> we'd have
>> 
>> COPY pg_largeobject_metadata FROM STDIN;
>> ...
>> 2121 10  {postgres=rw/postgres,joe=rw/postgres}
>> ...
>> 
>> and some appropriate COPY data for pg_shdepend too.

I did some more research here.  For many large objects without ACLs to
dump, I noticed that the vast majority of time is going to restoring the
ALTER OWNER commands.  For 1 million such large objects, restoring took ~73
seconds on my machine.  If I instead invented an lo_create_with_owner()
function and created 100 per SELECT command, the same restore takes ~7
seconds.  Copying the relevant pg_shdepend rows out and back in takes ~2.5
seconds.  I imagine using COPY for pg_largeobject_metadata would also take
a couple of seconds in this case.

For upgrading, I don't think there's any huge benefit to optimizing the
restore commands versus using COPY.  It might make future catalog changes
for large object stuff easier, but I'd expect those to be rare.  However,
the optimized restore commands could be nice for non-pg_upgrade use-cases.

-- 
nathan

Re: type cache cleanup improvements

2025-04-11 Thread Noah Misch

On Tue, Oct 22, 2024 at 08:33:24PM +0300, Alexander Korotkov wrote:
> On Tue, Oct 22, 2024 at 6:10 PM Pavel Borisov  wrote:
> > On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov  
> > wrote:
> >> I'm going to push this if no objections.

(This became commit b85a9d0.)

> + /* Call delete_rel_type_cache() if we actually cleared something */
> + if (hadTupDescOrOpclass)
> + delete_rel_type_cache_if_needed(typentry);

I think the intent was to maintain the invariant that a RelIdToTypeIdCacheHash
entry exists if and only if certain kinds of data appear in the TypeCacheHash
entry.  However, TypeCacheOpcCallback() clears TCFLAGS_OPERATOR_FLAGS without
maintaining RelIdToTypeIdCacheHash.  Is it right to do that?

Re: Fixing various typos in comments and docs

2025-04-11 Thread Daniel Gustafsson

> On 3 Mar 2025, at 01:39, Jacob Brazeal  wrote:
> 
> This patch fixes various typos I've found, most of them from recent commits.

Thanks, I've applied the fixes for typos introduced during the v18 cycle.  I
did leave a few out from your patch though:

> - Because not all statistics are not transferred by
> + Because not all statistics are transferred by

I skipped this as it changes the sentence completely rather than fix a typo.
It should perhaps still be fixed but not as part of a typo cleanup.

- * Many thanks to Adisak Pochanayon, who's article about 
SLZ
+ * Many thanks to Adisak Pochanayon, whose article about 
SLZ
This particular case is Jan's personal writing and not documentation so I don't
think we should change that.  The other instance of "who's" is probably a
correct fix but since that's an old typo it would require backpatching to avoid
risking conflicts for backpatching surrounding code so I left that one out as
well.

> Separately from this, I have been working on some tooling to flag typos in 
> new commits. Is that something we'd ever want to automate?

Existing spellcheckers for code usually have quite high rates of false
positives, so any automated tooling would have to avoid that to not become a
burden rather than a help.  Personally I think it's something which is best
suited for manual processing with manual review of findings, much like static
code analysis.

--
Daniel Gustafsson

New committer: Jacob Champion

2025-04-11 Thread Jonathan S. Katz

The Core Team would like to extend our congratulations to Jacob 
Champion, who has accepted an invitation to become our newest PostgreSQL 
committer.


Please join us in wishing Jacob much success and few reverts!

Thanks,

Jonathan


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: New committer: Jacob Champion

2025-04-11 Thread Joe Conway


On 4/11/25 16:26, Jonathan S. Katz wrote:

The Core Team would like to extend our congratulations to Jacob
Champion, who has accepted an invitation to become our newest PostgreSQL
committer.

Please join us in wishing Jacob much success and few reverts!


\o/

Congrats Jacob!

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

Re: New committer: Jacob Champion

On Fri, Apr 11, 2025 at 01:26:04PM -0700, Jonathan S. Katz wrote:
> The Core Team would like to extend our congratulations to Jacob Champion,
> who has accepted an invitation to become our newest PostgreSQL committer.
> 
> Please join us in wishing Jacob much success and few reverts!

Congrats!

-- 
nathan

Re: Large expressions in indexes can't be stored (non-TOASTable)

On Wed, Apr 09, 2025 at 08:54:21PM -0300, Euler Taveira wrote:
> LGTM. I have a few suggestions.

Thanks for reviewing.

> +   /*
> +* To avoid needing a TOAST table for pg_replication_origin, we restrict
> +* replication origin names to 512 bytes.  This should be more than enough
> +* for all practical use.
> +*/
> +   if (strlen(roname) > MAX_RONAME_LEN)
> +   ereport(ERROR,
> 
> I wouldn't duplicate the comment. Instead, I would keep it only in origin.h.

Hm.  I agree that duplicating the comment isn't great, but I'm also not
wild about forcing readers to jump to the macro definition to figure out
why there is a length restriction.

> +errdetail("Repilcation origin names must be no longer than 
> %d bytes.",
> +  MAX_RONAME_LEN)));
> 
> s/Repilcation/Replication/

Fixed.

> +#define MAX_RONAME_LEN (512)
> 
> It is just a cosmetic suggestion but I usually use parentheses when it is an
> expression but don't include it for a single number.

We use both styles, but the no-parentheses style does seem to be preferred.

$ grep -E "^#define\s[A-Z_]+\s\([0-9]+\)$" src/* -rI | wc -l
  23
$ grep -E "^#define\s[A-Z_]+\s[0-9]+$" src/* -rI | wc -l
 861

> Should we document this maximum length?

I've added a note.

-- 
nathan
>From 6762a3786b76379c82ccfa4c9da1812e928179b5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Wed, 9 Apr 2025 14:00:31 -0500
Subject: [PATCH v2 1/1] Remove pg_replication_origin's TOAST table.

---
 doc/src/sgml/func.sgml   |  1 +
 src/backend/catalog/catalog.c|  2 --
 src/backend/replication/logical/origin.c | 12 
 src/include/catalog/pg_replication_origin.h  |  2 --
 src/include/replication/origin.h |  7 +++
 src/test/regress/expected/misc_functions.out |  4 
 src/test/regress/expected/misc_sanity.out|  6 +-
 src/test/regress/sql/misc_functions.sql  |  3 +++
 src/test/regress/sql/misc_sanity.sql |  3 +++
 9 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 1c5cfee25d1..a9c5c855524 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -29941,6 +29941,7 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * 
ps.setting::int + :offset

 Creates a replication origin with the given external
 name, and returns the internal ID assigned to it.
+The name must be no longer than 512 bytes.

   
 
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index a6edf614606..a7eb60dd2d2 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -315,8 +315,6 @@ IsSharedRelation(Oid relationId)
relationId == PgDbRoleSettingToastIndex ||
relationId == PgParameterAclToastTable ||
relationId == PgParameterAclToastIndex ||
-   relationId == PgReplicationOriginToastTable ||
-   relationId == PgReplicationOriginToastIndex ||
relationId == PgShdescriptionToastTable ||
relationId == PgShdescriptionToastIndex ||
relationId == PgShseclabelToastTable ||
diff --git a/src/backend/replication/logical/origin.c 
b/src/backend/replication/logical/origin.c
index 6583dd497da..c17c0348c6e 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -264,6 +264,18 @@ replorigin_create(const char *roname)
SysScanDesc scan;
ScanKeyData key;
 
+   /*
+* To avoid needing a TOAST table for pg_replication_origin, we restrict
+* replication origin names to 512 bytes.  This should be more than 
enough
+* for all practical use.
+*/
+   if (strlen(roname) > MAX_RONAME_LEN)
+   ereport(ERROR,
+   (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+errmsg("replication origin name is too long"),
+errdetail("Replication origin names must be no 
longer than %d bytes.",
+  MAX_RONAME_LEN)));
+
roname_d = CStringGetTextDatum(roname);
 
Assert(IsTransactionState());
diff --git a/src/include/catalog/pg_replication_origin.h 
b/src/include/catalog/pg_replication_origin.h
index deb43065fe9..7ade8bbda39 100644
--- a/src/include/catalog/pg_replication_origin.h
+++ b/src/include/catalog/pg_replication_origin.h
@@ -54,8 +54,6 @@ 
CATALOG(pg_replication_origin,6000,ReplicationOriginRelationId) BKI_SHARED_RELAT
 
 typedef FormData_pg_replication_origin *Form_pg_replication_origin;
 
-DECLARE_TOAST_WITH_MACRO(pg_replication_origin, 4181, 4182, 
PgReplicationOriginToastTable, PgReplicationOriginToastIndex);
-
 DECLARE_UNIQUE_INDEX_PKEY(pg_replication_origin_roiident_index, 6001, 
ReplicationOriginIdentIndex, pg_replication_origin

Re: MergeJoin beats HashJoin in the case of multiple hash clauses

2025-04-11 Thread Alexander Korotkov

On Fri, Apr 11, 2025 at 5:06 AM Andres Freund  wrote:
> On 2025-04-11 00:47:19 +0200, Matthias van de Meent wrote:
> > On Fri, 11 Apr 2025 at 00:27, Andres Freund  wrote:
> > >
> > > Hi,
> > >
> > > On 2025-03-09 14:13:52 +0200, Alexander Korotkov wrote:
> > > > I've revised commit message, comments, formatting etc.
> > > > I'm going to push this if no objections.
> > >
> > > I'm rather confused as to why this is a thing to push at this point? This
> > > doesn't seem to be a bugfix and it's post feature freeze.
> >
> > I think the patch from that mail got committed as 6bb6a62f about a
> > month ago, which was shortly after Alexander's message. Did you get
> > confused about the month of his message, or by the incorrect state of
> > the CF entry?
>
> Sorry for that Alexander - for some reason the email just showed up as new in
> my inbox and I only looked at the date, not the month :(

Not a problem at all!

--
Regards,
Alexander Korotkov
Supabase

Re: type cache cleanup improvements

2025-04-11 Thread Alexander Korotkov

On Fri, Apr 11, 2025 at 11:32 PM Noah Misch  wrote:
>
> On Tue, Oct 22, 2024 at 08:33:24PM +0300, Alexander Korotkov wrote:
> > On Tue, Oct 22, 2024 at 6:10 PM Pavel Borisov  
> > wrote:
> > > On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov  
> > > wrote:
> > >> I'm going to push this if no objections.
>
> (This became commit b85a9d0.)
>
> > + /* Call delete_rel_type_cache() if we actually cleared something */
> > + if (hadTupDescOrOpclass)
> > + delete_rel_type_cache_if_needed(typentry);
>
> I think the intent was to maintain the invariant that a RelIdToTypeIdCacheHash
> entry exists if and only if certain kinds of data appear in the TypeCacheHash
> entry.  However, TypeCacheOpcCallback() clears TCFLAGS_OPERATOR_FLAGS without
> maintaining RelIdToTypeIdCacheHash.  Is it right to do that?

Thank you for the question.  I'll recheck this in next couple of days.

--
Regards,
Alexander Korotkov
Supabase

Re: n_ins_since_vacuum stats for aborted transactions

On Fri, Apr 11, 2025 at 12:33 PM Sami Imseih  wrote:

>
> > I'm also thinking to reword n_tup_upd, something like:
> >
> > Total number of rows updated.  Subsets of these updates are also tracked
> in n_tup_hot_upd and n_tup_newpage_upd to facilitate performance monitoring.
>
> I think the current explanation is clear enough, I am also not too
> thrilled about the "...to facilitate performance monitoring." since
> the cumulative stats system
> as a whole is known to be used to facilitate perf monitoring.
>

Yeah, it was mostly a style thing - I was trying to avoid using
parentheses, but the existing does make the needed point.


> What do you think of the attached?
>
>
WFM.  Though is there a reason to avoid adding the "why" of the exception
for n_mod_since_analyze?

David J.

Re: New committer: Jacob Champion

2025-04-11 Thread Michael Paquier

On Fri, Apr 11, 2025 at 01:26:04PM -0700, Jonathan S. Katz wrote:
> Please join us in wishing Jacob much success and few reverts!

Congratulations and welcome, Jacob!

May your path lead to a peaceful buildfarm and few reverts.
--
Michael


signature.asc
Description: PGP signature

Re: stats.sql fails during installcheck on mac

2025-04-11 Thread Michael Paquier

On Fri, Apr 11, 2025 at 10:44:59AM -0500, Sami Imseih wrote:
> I actually originally had it this way, but for some reason
> felt it would be better to be explicit about the methods we want to test 
> rather
> than not test. I can't think of a very compelling reason to go either way, so 
> v2
> LGTM.

I will proceed with v2 then, thanks.

> what do you think of this? I think we should set fsync = on
> at least for the part of the test that proceeds the 2 checkpoints and
> set if back to off at the end of the tests for fsync stats. It is concerning
> the tests for the fsync stats are not being exercised in
> the buildfarm.

One thing I fear here is the impact for animals with little capacity,
like PIs and the like.  On the other hand, I could just switch one of
my animals to use fsync = on on at least one branch.
--
Michael


signature.asc
Description: PGP signature

Re: New committer: Jacob Champion

2025-04-11 Thread Bharath Rupireddy

Congratulations Jacob!

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


On Fri, Apr 11, 2025 at 1:26 PM Jonathan S. Katz 
wrote:

> The Core Team would like to extend our congratulations to Jacob
> Champion, who has accepted an invitation to become our newest PostgreSQL
> committer.
>
> Please join us in wishing Jacob much success and few reverts!
>
> Thanks,
>
> Jonathan
>

Re: Fixing various typos in comments and docs

2025-04-11 Thread Jacob Brazeal

Thank you! I had completely forgotten about this, I appreciate that you dug
this one out of the archives!

> Existing spellcheckers for code usually have quite high rates of false
> positives, so any automated tooling would have to avoid that to not
become a
> burden rather than a help.  Personally I think it's something which is
best
>suited for manual processing with manual review of findings, much like
static
> code analysis.

Sounds good.

Re: n_ins_since_vacuum stats for aborted transactions

> WFM.  Though is there a reason to avoid adding the "why" of the exception for 
> n_mod_since_analyze?

I did think about that. I thought it will be understood that since
this is a field that deals with ANALYZE,
it will be understood that only committed changes matter here, and not
worth adding more text to the
description. but, maybe it's worth it?

--
Sami

Re: stats.sql fails during installcheck on mac

> > what do you think of this? I think we should set fsync = on
> > at least for the part of the test that proceeds the 2 checkpoints and
> > set if back to off at the end of the tests for fsync stats. It is concerning
> > the tests for the fsync stats are not being exercised in
> > the buildfarm.
>
> One thing I fear here is the impact for animals with little capacity,
> like PIs and the like.  On the other hand, I could just switch one of
> my animals to use fsync = on on at least one branch.

Yes, there should be some tests running for these stats,
so if it's possible to enable fsync on one or a few animals, that
will be better than nothing.

--
Sami

Re: Add missing PGDLLIMPORT markings


On 09.04.25 12:02, Peter Eisentraut wrote:
I ran src/tools/mark_pgdllimport.pl and it detected a few new global 
variables with missing markings.  See attached patch.  Please point out 
if any of these should not be marked or if they are special cases in 
some other way.  I'm Cc'ing various people involved with that new code.


I have committed the remaining ones.

Re: Improve a few appendStringInfo calls new to v18


On 10.04.25 05:51, David Rowley wrote:

Looks like v18 has grown a few appendStringInfo misusages, e.g. using
appendStringInfo() when no formatting is needed or just using format
"%s" instead of using appendStringInfoString().


Would it be useful to augment appendStringInfo() something like this:

if (VA_ARGS_NARGS() == 0)
return appendStringInfoString(str, fmt);

?

Re: n_ins_since_vacuum stats for aborted transactions

On Fri, Apr 11, 2025 at 5:19 PM Sami Imseih  wrote:

> > WFM.  Though is there a reason to avoid adding the "why" of the
> exception for n_mod_since_analyze?
>
> I did think about that. I thought it will be understood that since
> this is a field that deals with ANALYZE,
> it will be understood that only committed changes matter here, and not
> worth adding more text to the
> description. but, maybe it's worth it?
>
>
Absent field questions I'm content to think it is sufficiently obvious or
discoverable for others.

David J.

Re: New committer: Jacob Champion

2025-04-11 Thread Srinath Reddy

Congrats, Jacob.

Thanks && Regards,
Srinath Reddy Sadipiralla,
EDB: https://www.enterprisedb.com

Re: New committer: Jacob Champion

2025-04-11 Thread Amul Sul

On Saturday, 12 April 2025, Jonathan S. Katz  wrote:

> The Core Team would like to extend our congratulations to Jacob Champion,
> who has accepted an invitation to become our newest PostgreSQL committer.
>
> Please join us in wishing Jacob much success and few reverts!
>

Many congratulations, Jacob.

Regards,
Amul


-- 
Regards,
Amul Sul
EDB: http://www.enterprisedb.com

Re: stats.sql fails during installcheck on mac

2025-04-11 Thread Michael Paquier

On Fri, Apr 11, 2025 at 07:37:49PM -0500, Sami Imseih wrote:
> Yes, there should be some tests running for these stats,
> so if it's possible to enable fsync on one or a few animals, that
> will be better than nothing.

I have just done that on batta that only tests HEAD, that's a start.
--
Michael


signature.asc
Description: PGP signature

Small patch fixing a query correctness issue in Gin with operator classes implementing Consistent functions

2025-04-11 Thread Vinod Sridharan

Hi All,

Please find a small patch to fix an existing bug in the GIN index that
impacts correctness of query results for operator classes that use a
consistentFunction and do not specify a triConsistent function. This
patch is against the master and fixes an issue not in any release
branches.

Please find the thread discussing this issue and the fix here:
https://www.postgresql.org/message-id/flat/CAFMdLD4Ks5b%3DCbBh1PjFSytm0zdNv9-ddyeE%2BopusAKCVph7%3Dg%40mail.gmail.com

--
Thanks and Regards,
Vinod Sridharan
[Microsoft]


v1-0001-Fix-shimTriConsistentFn-mutating-the-entryRes-val.patch
Description: Binary data

Re: New committer: Jacob Champion

2025-04-11 Thread Peter Geoghegan

On Fri, Apr 11, 2025 at 4:26 PM Jonathan S. Katz  wrote:
> Please join us in wishing Jacob much success and few reverts!

Well done, Jacob.

-- 
Peter Geoghegan

Re: TOAST versus toast