Re: What's our minimum supported Python version?
On 06.06.25 17:46, Jacob Champion wrote: On Fri, Jun 6, 2025 at 7:17 AM Tom Lane wrote: Peter Eisentraut writes: Since we now require Python 3.6, we can also remove PL/Python test alternative expected files for earlier Python versions. See attached patch. +1. So nice to get rid of src/pl/plpython/expected/README. Awesome! LGTM. done
Re: Make tuple deformation faster
Hello David,
24.12.2024 03:57, David Rowley wrote:
On Tue, 24 Dec 2024 at 11:19, David Rowley wrote:
The attached adjusts that Assert code so that a fresh CompactAttribute
is populated instead of modifying the TupleDesc's one. I'm not sure
if populate_compact_attribute_internal() is exactly the nicest way to
do this. I'll think a bit harder about that. Assume the attached is
POC grade.
I've now pushed a fix for this using the same method but with the code
factored around a little differently. I didn't want to expose the
populate_compact_attribute_internal() function externally, so I
invented verify_compact_attribute() to call from
TupleDescCompactAttr().
I stumbled upon that assertion failure again. It's not reproduced easily,
but maybe you can forgive me the following modification:
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -159,8 +159,11 @@ verify_compact_attribute(TupleDesc tupdesc, int attnum)
tmp.attcacheoff = cattr->attcacheoff;
tmp.attnullability = cattr->attnullability;
+for (int i = 0; i < 1000; i++)
+{
/* Check the freshly populated CompactAttribute matches the TupleDesc's */
Assert(memcmp(&tmp, cattr, sizeof(CompactAttribute)) == 0);
+}
#endif
}
which helps for this script:
for i in {1..50}; do
echo "ITERATION $i"
for c in {1..20}; do
echo "
set parallel_setup_cost = 1;
set min_parallel_table_scan_size = '1kB';
select * from information_schema.role_udt_grants limit 50;
" | psql > psql-$c.log &
done
wait
grep 'was terminated by signal' server.log && break;
done
to fail for me as below:
...
ITERATION 34
WARNING: terminating connection because of crash of another server process
DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another
server process exited abnormally and possibly corrupted shared memory.
HINT: In a moment you should be able to reconnect to the database and repeat
your command.
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
connection to server was lost
2025-06-07 13:01:39.326 EEST [537106] LOG: background worker "parallel worker" (PID 539473) was terminated by signal 6:
Aborted
Core was generated by `postgres: parallel worker for PID 539434
'.
Program terminated with signal SIGABRT, Aborted.
(gdb) bt
#0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=)
at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=) at
./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=, signo=signo@entry=6) at
./nptl/pthread_kill.c:89
#3 0x7de05ec4526e in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4 0x7de05ec288ff in __GI_abort () at ./stdlib/abort.c:79
#5 0x5dd0a1788377 in ExceptionalCondition (conditionName=0x5dd0a1822ee8 "memcmp(&tmp, cattr,
sizeof(CompactAttribute)) == 0",
fileName=0x5dd0a1822ed9 "tupdesc.c", lineNumber=165) at assert.c:66
#6 0x5dd0a0f85bfd in verify_compact_attribute (tupdesc=0x7de05ef01000,
attnum=1) at tupdesc.c:165
#7 0x5dd0a0f72a25 in TupleDescCompactAttr (tupdesc=0x7de05ef01000, i=1) at
../../../../src/include/access/tupdesc.h:182
#8 0x5dd0a0f73da6 in nocachegetattr (tup=0x7ffc737cc550, attnum=1,
tupleDesc=0x7de05ef01000) at heaptuple.c:581
#9 0x5dd0a12719c9 in fastgetattr (tup=0x7ffc737cc550, attnum=2,
tupleDesc=0x7de05ef01000, isnull=0x5dd0a7b919d5)
at ../../../src/include/access/htup_details.h:880
#10 0x5dd0a1271a74 in heap_getattr (tup=0x7ffc737cc550, attnum=2,
tupleDesc=0x7de05ef01000, isnull=0x5dd0a7b919d5)
at ../../../src/include/access/htup_details.h:916
#11 0x5dd0a127a50d in ExecEvalFieldSelect (state=0x5dd0a7b919d0, op=0x5dd0a7b93258, econtext=0x5dd0a7b86648) at
execExprInterp.c:3837
#12 0x5dd0a12759ce in ExecInterpExpr (state=0x5dd0a7b919d0, econtext=0x5dd0a7b86648, isnull=0x0) at
execExprInterp.c:1698
#13 0x5dd0a127702f in ExecInterpExprStillValid (state=0x5dd0a7b919d0, econtext=0x5dd0a7b86648, isNull=0x0) at
execExprInterp.c:2299
#14 0x5dd0a12db079 in ExecEvalExprNoReturn (state=0x5dd0a7b919d0, econtext=0x5dd0a7b86648) at
../../../src/include/executor/executor.h:417
#15 0x5dd0a12db137 in ExecEvalExprNoReturnSwitchContext (state=0x5dd0a7b919d0, econtext=0x5dd0a7b86648) at
../../../src/include/executor/executor.h:458
#16 0x5dd0a12db198 in ExecProject (projInfo=0x5dd0a7b919c8) at
../../../src/include/executor/executor.h:490
#17 0x5dd0a12db3bb in ExecResult (pstate=0x5dd0a7b86538) at nodeResult.c:135
#18 0x5dd0a1290e47 in ExecProcNodeFirst (node=0x5dd0a7b86538) at
execProcnode.c:469
#19 0x5dd0a12e2823 in ExecProcNode (node=0x5dd0a7b86538) at
../../../src/include/executor/executor.h:313
#20 0x5dd0a12e2848 in SubqueryNext (node=0x5dd0a7b86318) at
nodeSubqueryscan.c:53
#21 0x5dd0a1295a36 in ExecScanFetch (node=0x5dd0
Re: Batch TIDs lookup in ambulkdelete
Hi Masahiko,
On Sat, Jun 7, 2025 at 5:34 AM Masahiko Sawada wrote:
>
> On Tue, May 13, 2025 at 2:26 PM Matheus Alcantara
> wrote:
> >
> > Hi,
> >
> > On 30/04/25 19:36, Masahiko Sawada wrote:
> > > Here are the summary of each attached patch:
> > >
> > > 0001: Introduce TIdStoreIsMemberMulti() where we can do IsMember check
> > > for multiple TIDs in one function call. If the given TIDs are sorted
> > > (at least in block number), we can save radix tree lookup for the same
> > > page entry.
> > >
> > > 0002: Convert IndexBUlkDeleteCallback() to batched operation.
> > >
> > > 0003: Use batch TIDs lookup in btree index bulk-deletion.
> > >
> > > In patch 0003, we implement batch TID lookups for both each
> > > deduplicated index tuple and remaining all regular index tuples, which
> > > seems to be the most straightforward approach. While further
> > > optimizations are possible, such as performing batch TID lookups for
> > > all index tuples on a single page, these could introduce additional
> > > overhead from sorting and re-sorting TIDs. Moreover, considering that
> > > radix tree lookups are relatively inexpensive, the benefits of sorting
> > > TIDs and using TidStoreIsMemberMulti() might be minimal. Nevertheless,
> > > these potential optimizations warrant further evaluation to determine
> > > their actual impact on performance.
> > >
> > > Also, the patch includes the batch TIDs lookup support only for btree
> > > indexes but we potentially can improve other index AMs too.
> > >
> >
> > The code looks good and also +1 for the idea. I just have some small
> > points:
> > - Maybe it would be good to mention somewhere that
> > IndexBulkDeleteCallback() callback returns the number of tids
> > members found on TidStore?
> > - The vac_tid_reaped() docs may need to be updated?
>
> Thank you for looking at the patches. I agree with the above comments.
>
> >
> > I also executed meson tests for each patch individually and the 0002
> > patch is not passing on my machine (MacOs).
> >
> > Ok: 39
> > Expected Fail: 0
> > Fail: 271
> > Unexpected Pass:0
> > Skipped:22
> > Timeout:0
> >
> > One behaviour that I found by executing the 0002 tests is that it may be
> > leaking some shared memory segments. I notice that because after
> > executing the tests I tried to re-execute based on master and all tests
> > were failing with the "Failed system call was shmget(key=97530599,
> > size=56, 03600)" error. I also checked the shared memory segments using
> > "ipcs -m" and it returns some segments which is don't returned when I
> > execute the tests on master (after cleaning the leaked memory segments)
> > and it also doesn't occur when executing based on 0001 or 0003.
> >
> > ~/d/p/batch-tids-lookup-ambulkdelete ❯❯❯ ipcs -m
> > IPC status from as of Tue May 13 18:19:14 -03 2025
> > T ID KEYMODE OWNERGROUP
> > Shared Memory:
> > m 18087936 0x05f873bf --rw--- matheusstaff
> > m 15925250 0x05f966fe --rw--- matheusstaff
> > m 24248325 0x05f9677e --rw--- matheusstaff
> >
> >
> > Note that the the 0003 patch don't have this issue so at the end we will
> > not have problem with this I think, but it maybe be good to mention that
> > although the patches are separate, there is a dependency between them,
> > which may cause issues on buildfarm?
>
> Thank you for the report. With the 0001 and 0002 patches, I got a
> SEGV. I've fixed this issue in the attached updated version patches.
> I've confirmed that the patches pass CI tests but I'm not sure it
> fixes the shared memory segment leak problem you reported. The
> attached patches incorporated the comments[1] from John as well.
>
> BTW I found that the constant 'maxblkno' in test_tidstore.sql actually
> equals to InvalidBlockNumber, but not MaxBlockNumber. I think it
> doesn't make sense that TidStore uses InvalidBlockNumber as the key.
> The attached 0001 patch fixes it. I think we can fix it separately on
> HEAD as well as back branches.
>
> Regards,
>
> [1]
> https://www.postgresql.org/message-id/canwcazbijcwsbcczlfbfipe1met+v9pjtzv5vvubwarlvx1...@mail.gmail.com
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
+ /*
+ * We will sort the deletable array if there are existing
+ * offsets as it should be sorted in ascending order (see
+ * _bt_delitems_vacuum()).
+ */
+ need_sort = (ndeletable > 0);
+
+ ndels = callback(workbuf_htids, workbuf_nitem, workbuf_deletable,
+ callback_state);
+ if (ndels > 0)
+ {
+ for (int i = 0; i < workbuf_nitem; i++)
+ {
+ if (workbuf_deletable[i])
+ deletable[ndeletable++] = workbuf_offs[i];
+ }
+
+ if (need_sort)
+ qsort(deletable, ndeletable, sizeof(OffsetNumber), cmpOffsetNumbers);
+
+ nhtidsdead += ndels;
+ }
I think the need_sort should be calculated after the callback? Maybe just:
if (ndeletable > 1)
qsort(deletable, ndeletable, sizeof(OffsetNumber), cmpOffsetNumbers);
I thi
Re: Batch TIDs lookup in ambulkdelete
On Sat, Jun 7, 2025 at 8:46 PM Junwang Zhao wrote:
>
> Hi Masahiko,
>
> On Sat, Jun 7, 2025 at 5:34 AM Masahiko Sawada wrote:
> >
> > On Tue, May 13, 2025 at 2:26 PM Matheus Alcantara
> > wrote:
> > >
> > > Hi,
> > >
> > > On 30/04/25 19:36, Masahiko Sawada wrote:
> > > > Here are the summary of each attached patch:
> > > >
> > > > 0001: Introduce TIdStoreIsMemberMulti() where we can do IsMember check
> > > > for multiple TIDs in one function call. If the given TIDs are sorted
> > > > (at least in block number), we can save radix tree lookup for the same
> > > > page entry.
> > > >
> > > > 0002: Convert IndexBUlkDeleteCallback() to batched operation.
> > > >
> > > > 0003: Use batch TIDs lookup in btree index bulk-deletion.
> > > >
> > > > In patch 0003, we implement batch TID lookups for both each
> > > > deduplicated index tuple and remaining all regular index tuples, which
> > > > seems to be the most straightforward approach. While further
> > > > optimizations are possible, such as performing batch TID lookups for
> > > > all index tuples on a single page, these could introduce additional
> > > > overhead from sorting and re-sorting TIDs. Moreover, considering that
> > > > radix tree lookups are relatively inexpensive, the benefits of sorting
> > > > TIDs and using TidStoreIsMemberMulti() might be minimal. Nevertheless,
> > > > these potential optimizations warrant further evaluation to determine
> > > > their actual impact on performance.
> > > >
> > > > Also, the patch includes the batch TIDs lookup support only for btree
> > > > indexes but we potentially can improve other index AMs too.
> > > >
> > >
> > > The code looks good and also +1 for the idea. I just have some small
> > > points:
> > > - Maybe it would be good to mention somewhere that
> > > IndexBulkDeleteCallback() callback returns the number of tids
> > > members found on TidStore?
> > > - The vac_tid_reaped() docs may need to be updated?
> >
> > Thank you for looking at the patches. I agree with the above comments.
> >
> > >
> > > I also executed meson tests for each patch individually and the 0002
> > > patch is not passing on my machine (MacOs).
> > >
> > > Ok: 39
> > > Expected Fail: 0
> > > Fail: 271
> > > Unexpected Pass:0
> > > Skipped:22
> > > Timeout:0
> > >
> > > One behaviour that I found by executing the 0002 tests is that it may be
> > > leaking some shared memory segments. I notice that because after
> > > executing the tests I tried to re-execute based on master and all tests
> > > were failing with the "Failed system call was shmget(key=97530599,
> > > size=56, 03600)" error. I also checked the shared memory segments using
> > > "ipcs -m" and it returns some segments which is don't returned when I
> > > execute the tests on master (after cleaning the leaked memory segments)
> > > and it also doesn't occur when executing based on 0001 or 0003.
> > >
> > > ~/d/p/batch-tids-lookup-ambulkdelete ❯❯❯ ipcs -m
> > > IPC status from as of Tue May 13 18:19:14 -03 2025
> > > T ID KEYMODE OWNERGROUP
> > > Shared Memory:
> > > m 18087936 0x05f873bf --rw--- matheusstaff
> > > m 15925250 0x05f966fe --rw--- matheusstaff
> > > m 24248325 0x05f9677e --rw--- matheusstaff
> > >
> > >
> > > Note that the the 0003 patch don't have this issue so at the end we will
> > > not have problem with this I think, but it maybe be good to mention that
> > > although the patches are separate, there is a dependency between them,
> > > which may cause issues on buildfarm?
> >
> > Thank you for the report. With the 0001 and 0002 patches, I got a
> > SEGV. I've fixed this issue in the attached updated version patches.
> > I've confirmed that the patches pass CI tests but I'm not sure it
> > fixes the shared memory segment leak problem you reported. The
> > attached patches incorporated the comments[1] from John as well.
> >
> > BTW I found that the constant 'maxblkno' in test_tidstore.sql actually
> > equals to InvalidBlockNumber, but not MaxBlockNumber. I think it
> > doesn't make sense that TidStore uses InvalidBlockNumber as the key.
> > The attached 0001 patch fixes it. I think we can fix it separately on
> > HEAD as well as back branches.
> >
> > Regards,
> >
> > [1]
> > https://www.postgresql.org/message-id/canwcazbijcwsbcczlfbfipe1met+v9pjtzv5vvubwarlvx1...@mail.gmail.com
> >
> > --
> > Masahiko Sawada
> > Amazon Web Services: https://aws.amazon.com
>
> + /*
> + * We will sort the deletable array if there are existing
> + * offsets as it should be sorted in ascending order (see
> + * _bt_delitems_vacuum()).
> + */
> + need_sort = (ndeletable > 0);
> +
> + ndels = callback(workbuf_htids, workbuf_nitem, workbuf_deletable,
> + callback_state);
> + if (ndels > 0)
> + {
> + for (int i = 0; i < workbuf_nitem; i++)
> + {
> + if (workbuf_deletable[i])
> + deletable[ndeletable++] = workbuf_offs[i];
> + }
> +
> + if (n
Re: Non-reproducible AIO failure
On 06/06/2025 10:21 pm, Tom Lane wrote: Konstantin Knizhnik writes: There is really essential difference in code generated by clang 15 (working) and 16 (not working). It's a mistake to think that this is a compiler bug. The C standard explicitly allows compilers to use word-wide operations to access bit-field struct members. Sorry, I have not said that I have found compiler bug. The generated code (clang 16) overwrites all three bitfields but looks like is doing it in correct way (old values of state and target are preserved). My first thought was that compiler moves forward assignment of state from `pgaio_io_update_state`. But it is not true: it just reads and writes old value of `state` field. It is also possible to expect that `pgaio_io_update_state` somehow "resurrects" old value of `op` field. But it is also not true: ``` postgres`pgaio_io_update_state: ... postgres[0x100699070] <+372>: dmb ish ; memory write barrier postgres[0x100699074] <+376>: ldur w10, [x29, #-0xc] ; w10 = state postgres[0x100699078] <+380>: ldur x9, [x29, #-0x8] ; x9 = ioh postgres[0x10069907c] <+384>: ldrh w8, [x9] ; w8 = ioh->state, ioh->target postgres[0x100699080] <+388>: ldrb w11, [x9, #0x2] ; w11 = ioh->op postgres[0x100699084] <+392>: orr w8, w8, w11, lsl #16 ; w8 = ioh->state, ioh->target, ioh->op postgres[0x100699088] <+396>: and w10, w10, #0xff ; w10 &= 0xff postgres[0x10069908c] <+400>: and w8, w8, #0xff00 ; w8 &= ~0xff postgres[0x100699090] <+404>: orr w10, w8, w10 ; w10 = state, ioh->target, ioh->op postgres[0x100699094] <+408>: lsr w8, w10, #16 ; w8 = ioh->op postgres[0x100699098] <+412>: strh w10, [x9] postgres[0x10069909c] <+416>: strb w8, [x9, #0x2] postgres[0x1006990a0] <+420>: ldp x29, x30, [sp, #0x60] postgres[0x1006990a4] <+424>: add sp, sp, #0x70 postgres[0x1006990a8] <+428>: ret ```
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
On Fri, Jun 6, 2025 at 8:04 PM David Rowley wrote: > On Fri, 6 Jun 2025 at 14:32, Robert Treat wrote: > > In production, you aren't watching to see what happen with > > pg_stat_all_indexes, because you will first be watching pg_stat_activity to > > see if the plans have flipped in some way that leads to an overloaded > > server (extra latency, poor caching effects, extra buffers usage, etc). And > > the replicated bit? Sadly someone launched some big DML operation so you're > > waiting for that to finish so the "quick rollback" can actually get to > > those other servers. > > I think you've misunderstood when you'd be looking at > pg_stat_all_indexes. The time when you'd want to look at > pg_stat_all_indexes is *before* you DROP INDEX and before you ALTER > TABLE INVISIBLE the index. What you'd likely want to look for there > are indexes that have the last_idx_scan set to something far in the > past or set to NULL. > I guess you have never heard of the TREAT method of index management? :-D - Test for duplicate indexes - Reindex bloated indexes - Eliminate unused indexes - Add missing indexes - Tune indexes for generic queries The easy part of figuring out what to change, the hard part (sometimes) is getting those changes into production safely; that's the part I am focused on. > I'm curious to know if you've ever had to drop an index out of > production before? What did you think about when you'd just typed the > DROP INDEX command and were contemplating your future? How long did > you pause before pressing [Enter]? > ROFL... Uh... yes, I have had to do it at least a few times. So, years ago I used to say things like "I wish we had a way to make indexes invisible like they do in Oracle" on the regular; but as I worked through several different implementations and their potential effects, and had more and more exposure to more demanding Postgres installations, my thinking evolved. I spoke with Sami a bit about this off-list and he walked me through some of the Oracle documentation on this (I had, at best, forgot the specifics), which I think was helpful to better understand some of the allure of the alter index/guc method for many people who are used to it (and this current version of the implementation is very Oracle like), but it also crystalized my feeling that an Oracle-style implementation would be a red herring that can keep us from a better solution. > Can you list your proposed series of steps you'd recommend to a DBA > wishing to remove an index, assuming this feature exists in core as > you'd like it to? > Well, the series of steps differs depending on the nature of the system being managed. If you are running on a single node with normal traffic and resources, you just set the GUC to include the index you want to be invisible, wait for a few days (maybe no one runs monthly reports on this system?), take a quick look at your monitoring/stats to make sure things seem copacetic, and then you drop the index and reset the GUC. But of course the people who I am most worried about are the ones who are operating on high scale, high transaction, high connection, "mission critical" systems... ie. people operating in high risk environments, where things can go very bad very fast. Where safety considerations are a critical part of every deployment. In that type of environment, the GUC-only method enables you to control changes at very precise levels, so you can do things like: - run it ad-hoc at the session level to confirm that the explain plans you get in production match your expectations. - you can stay ad-hoc at the session level and run explain analyze and confirm acceptable performance within your workload, and see what kind of buffer impact you are going to have (typically overlooked, but a potential landmine for outages, but I'll come back to this) - because we are operating at the session level, we can then add this on a per query basis at the application level, and in really high traffic scenarios, you can use canary releases and/or feature flags to ramp up those new queries into the live system. - depending on how much risk you are concerned about, you can use this session level method across queries individually, or at some point roll it up to a user/application level. And again, we can roll it out to different users at different times if you want. - at some point when you feel confident that you have covered enough angles, you set the GUC globally and let that marinate for a few more weeks as needed. And the funny thing is, at this point, once you have the guc put in globally, and it's run for some number of weeks or months and everyone is confident, you don't actually need the ALTER INDEX part any more; you can just drop the index and be done with it. Now of course if you aren't running at this kind of scale or don't have this level of risk, you can speed run this a bit and go directly to the user level or skip right to adding it globally, so the ease of use is on par with us
Re: why there is not VACUUM FULL CONCURRENTLY?
On Fri, Apr 11, 2025 at 5:28 PM Antonin Houska wrote:
>
> Please check the next version [1]. Thanks for your input.
>
> [1] https://www.postgresql.org/message-id/97795.1744363522%40localhost
>
Hi, I’ve briefly experimented with v13-0001.
EXPLAIN tab complete:
explain (verbose O
OFF ON
since we already touched the tab-complete for repack.
We can do it similarly.
you may see src/bin/psql/tab-complete.in.c line 4288.
--
Currently REPACK Synopsis section
looks like the screenshot attached.
make it one line
REPACK [ ( option [, ...] ) ] [ table_name [ USING INDEX index_name ] ]
would look intuitive, IMHO.
--
+repack_index_specification:
+ USING INDEX name { $$ = $3; }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
in gram.y line 4685, we have
ExistingIndex: USING INDEX name{ $$ = $3; }
;
so here, we can change it to
repack_index_specification:
ExistingIndex
| /*EMPTY*/{ $$ = NULL; }
-
+static List *
+get_tables_to_repack(MemoryContext repack_context)
+{
+ relrelation = table_open(RelationRelationId, AccessShareLock);
+ scan = table_beginscan_catalog(relrelation, 0, NULL);
+ while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+ {
+ RelToCluster *rtc;
+ Form_pg_class relrelation = (Form_pg_class) GETSTRUCT(tuple);
+ Oid relid = relrelation->oid;
+
+ /* Only interested in relations. */
+ if (get_rel_relkind(relid) != RELKIND_RELATION)
+ continue;
The doc said (Without a table_name, REPACK processes every table and
materialized view...)
but seems plain ``REPACK(verbose) ; ``
will not process materialized view?
Re: PG 18 release notes draft committed
On Thu, Jun 5, 2025 at 08:32:44PM -0400, Bruce Momjian wrote: > On Wed, Jun 4, 2025 at 05:53:38PM -0400, Bruce Momjian wrote: > > On Wed, Jun 4, 2025 at 02:29:46PM -0700, Noah Misch wrote: > > > I agree with David G. Johnston's feedback on this. My draft didn't > > > mention > > > SECURITY DEFINER, because I consider it redundant from a user's > > > perspective. > > > If a function is SECURITY DEFINER, that always overrides other sources of > > > user > > > identity. No need to mention it each time. > > > > Well, if it is a SECURITY DEFINER function, it is not going to be run as > > the user who is active at commit/execution time, so I think we have to > > specify that. > > I came up with this text: > > Execute AFTER triggers as the role that was active when trigger > events were queued > > Previously such triggers were run as the role that was active at > trigger execution time (e.g., at COMMIT). This is significant > for cases where the role is changed between queue time and > transaction commit. Item added to the incompatibilities section of the release notes. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future.
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
> In that type of environment, the GUC-only method enables you to > control changes at very precise levels, so you can do things like: > - run it ad-hoc at the session level to confirm that the explain plans > you get in production match your expectations. > - you can stay ad-hoc at the session level and run explain analyze and > confirm acceptable performance within your workload, and see what kind > of buffer impact you are going to have (typically overlooked, but a > potential landmine for outages, but I'll come back to this) > - because we are operating at the session level, we can then add this > on a per query basis at the application level, and in really high > traffic scenarios, you can use canary releases and/or feature flags to > ramp up those new queries into the live system. > - depending on how much risk you are concerned about, you can use this > session level method across queries individually, or at some point > roll it up to a user/application level. And again, we can roll it out > to different users at different times if you want. > - at some point when you feel confident that you have covered enough > angles, you set the GUC globally and let that marinate for a few more > weeks as needed. Do we need this level of granular control in core, or should this be delegated to other tools in the ecosystem, like pg_hint_plan? The de facto tool for influencing planning. There is probably some work that must happen in that extension to make the use-cases above work, but it is something to consider. With that said, I am not really opposed to a multi-value GUC that takes in a list of index names, but I do have several concerns with that approach being available in core: 1. The list of indexes getting too long, and the potential performance impact of having to translate the index name to a relid to find which index to make "invisible". I don't think a list of index relids will be good from a usability perspective either. 2. A foot-gun such as adding an index name to my list, dropping the index, recreating it with the same name, and now my new index is not being used. 3. not sync'd up with the replica, so manual work is required there. That could be seen as a positive aspect of this approach as well. 4. The above points speak on the level of maintenance required for this. > You mentioned that one of the things you liked about the ALTER/guc method > is that it replicates the changes across all systems which makes it > easy to revert, however I believe that thinking is flawed. For > starters, any change that has to occur across the WAL stream is not > something that can be relied on to happen quickly; there are too many > other items that traverse that space that could end up blocking a > rollback from being applied in a timely fashion. This is not going to be unique to this feature though. Other critical DDLs will be blocked, so this is a different problem, IMO. > but it also crystalized my > feeling that an Oracle-style implementation would be a red herring > that can keep us from a better solution. Going back to this point, I still think that the ALTER option is useful after the user's confidence is near 100% and they are ready to drop the index for good, and which also gets replicated. The GUC is useful for experimentation or for users that want to do a slow rollout of dropping an index. We can discuss whether this should be a multi-value setting or a boolean in core, or if it should be delegated to an extension. Essentially, I don't think we need to choose one or the other, but perhaps we can improve upon the GUC. -- Sami
Re: proposal: plpgsql, new check for extra_errors - strict_expr_check
Hi
only rebase
Regards
Pavel
From d589a48d746d05368ee49e6a1e0202da9b75b0f6 Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Sun, 16 Jun 2024 15:52:38 +0200
Subject: [PATCH 3/3] set plpgsql.extra_errors to "none"
Purpose of previous commit was to run tests with active strict_expr_check.
Now, reset to default and revert all changes from previous commit.
---
.../basic_archive/expected/basic_archive.out | 4 +--
contrib/basic_archive/sql/basic_archive.sql | 4 +--
src/pl/plpgsql/src/expected/plpgsql_array.out | 34 ---
src/pl/plpgsql/src/pl_handler.c | 4 +--
src/pl/plpgsql/src/sql/plpgsql_array.sql | 14
.../recovery/t/026_overwrite_contrecord.pl| 4 +--
src/test/regress/expected/alter_table.out | 2 +-
src/test/regress/expected/plancache.out | 2 +-
src/test/regress/expected/plpgsql.out | 12 +++
src/test/regress/expected/stats_ext.out | 2 +-
src/test/regress/expected/transactions.out| 4 +--
src/test/regress/sql/alter_table.sql | 2 +-
src/test/regress/sql/plancache.sql| 2 +-
src/test/regress/sql/plpgsql.sql | 12 +++
src/test/regress/sql/stats_ext.sql| 2 +-
src/test/regress/sql/transactions.sql | 4 +--
16 files changed, 52 insertions(+), 56 deletions(-)
diff --git a/contrib/basic_archive/expected/basic_archive.out b/contrib/basic_archive/expected/basic_archive.out
index 280ff3e022e..0015053e0f2 100644
--- a/contrib/basic_archive/expected/basic_archive.out
+++ b/contrib/basic_archive/expected/basic_archive.out
@@ -11,8 +11,8 @@ DECLARE
loops int := 0;
BEGIN
LOOP
- archived := (SELECT count(*) > 0 FROM pg_ls_dir('.', false, false) a
- WHERE a ~ '^[0-9A-F]{24}$');
+ archived := count(*) > 0 FROM pg_ls_dir('.', false, false) a
+ WHERE a ~ '^[0-9A-F]{24}$';
IF archived OR loops > 120 * 10 THEN EXIT; END IF;
PERFORM pg_sleep(0.1);
loops := loops + 1;
diff --git a/contrib/basic_archive/sql/basic_archive.sql b/contrib/basic_archive/sql/basic_archive.sql
index 2c127a821f1..14e236d57ab 100644
--- a/contrib/basic_archive/sql/basic_archive.sql
+++ b/contrib/basic_archive/sql/basic_archive.sql
@@ -7,8 +7,8 @@ DECLARE
loops int := 0;
BEGIN
LOOP
- archived := (SELECT count(*) > 0 FROM pg_ls_dir('.', false, false) a
- WHERE a ~ '^[0-9A-F]{24}$');
+ archived := count(*) > 0 FROM pg_ls_dir('.', false, false) a
+ WHERE a ~ '^[0-9A-F]{24}$';
IF archived OR loops > 120 * 10 THEN EXIT; END IF;
PERFORM pg_sleep(0.1);
loops := loops + 1;
diff --git a/src/pl/plpgsql/src/expected/plpgsql_array.out b/src/pl/plpgsql/src/expected/plpgsql_array.out
index caf07e834e5..4c6b3ce998a 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_array.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_array.out
@@ -50,7 +50,7 @@ do $$ declare a quadarray;
begin a.c1[1].i := 11; raise notice 'a = %, a.c1[1].i = %', a, a.c1[1].i; end$$;
NOTICE: a = ("{""(,11)""}",), a.c1[1].i = 11
do $$ declare a int[];
-begin a := (select array_agg(x) from (values(1),(2),(3)) v(x)); raise notice 'a = %', a; end$$;
+begin a := array_agg(x) from (values(1),(2),(3)) v(x); raise notice 'a = %', a; end$$;
NOTICE: a = {1,2,3}
do $$ declare a int[] := array[1,2,3];
begin
@@ -64,34 +64,30 @@ end$$;
NOTICE: a = {1,1,2,3,42,3,1,1,2,3,42,3}
create temp table onecol as select array[1,2] as f1;
do $$ declare a int[];
-begin a := (select f1 from onecol); raise notice 'a = %', a; end$$;
+begin a := f1 from onecol; raise notice 'a = %', a; end$$;
NOTICE: a = {1,2}
do $$ declare a int[];
-begin a := (select * from onecol for update); raise notice 'a = %', a; end$$;
+begin a := * from onecol for update; raise notice 'a = %', a; end$$;
NOTICE: a = {1,2}
-- error cases:
do $$ declare a int[];
-begin a := (select from onecol); raise notice 'a = %', a; end$$;
-ERROR: subquery must return only one column
-LINE 1: a := (select from onecol)
- ^
-QUERY: a := (select from onecol)
-CONTEXT: PL/pgSQL function inline_code_block line 2 at assignment
+begin a := from onecol; raise notice 'a = %', a; end$$;
+ERROR: assignment source returned 0 columns
+CONTEXT: PL/pgSQL assignment "a := from onecol"
+PL/pgSQL function inline_code_block line 2 at assignment
do $$ declare a int[];
-begin a := (select f1, f1 from onecol); raise notice 'a = %', a; end$$;
-ERROR: subquery must return only one column
-LINE 1: a := (select f1, f1 from onecol)
- ^
-QUERY: a := (select f1, f1 from onecol)
-CONTEXT: PL/pgSQL function inline_code_block line 2 at assignment
+begin a := f1, f1 from onecol; raise notice 'a = %', a; end$$;
+ERROR: assignment source returned 2 columns
+CONTEXT: PL/pgSQL assignment "a := f1, f1 from onecol"
+PL/pgSQL function inline_code_block line 2 at assignment
insert into onecol values(array[11]);
do $$ declare a int[];
-begin a := (select f1 from onecol); raise notice 'a = %', a; end$$;
-ERROR: more than one row returned by a subquery used a
Enhancing PostgreSQL Management and Observability in Cloud Environments
Dear PostgreSQL Community, I hope this message finds you well. I am writing to propose a set of features aimed at significantly enhancing PostgreSQL’s management and observability in cloud environments, particularly for administrators who do not have direct access to the underlying operating system of database servers. These suggestions focus on improving security auditing, backup tracking, system metrics visibility, and configuration management—all through SQL-level commands. 1. Dynamic pg_hba.conf Management Proposal: Introduce SQL-based commands to dynamically manage pg_hba.conf entries, removing the need for manual file edits. For example: ALTER SYSTEM ADD PG_HBA ( type => 'host', database => 'mydb', user => 'myuser', address => '192.168.1.0/24', method => 'md5', comment => 'This connection from app_prod' ); Benefits: • Enables safe, cloud-based management of the pg_hba.conf file • Supports dynamic configuration without requiring direct OS access • Ideal for DBaaS environments where administrators may not have OS-level privileges 2. Login Monitoring Proposal: Tracks login attempts and offers key insights into authentication status. Key fields might include: • user: Database user attempting to login • db_name: Database name • client_address: IP address of the client • client_application: The application from which the connection originated • last_attempt_time: Timestamp of the last login attempt • last_attempt_status: Outcome of the last login attempt (e.g., success, failed) • connection_status_remarks: Detailed remarks on the connection (e.g., no match in pg_hba.conf, SSL-related errors, etc.) Benefits: • Facilitates detailed security auditing of login attempts • Helps troubleshoot authentication problems • Provides insights into connection patterns and potential issues • Enhances monitoring and security in DBaaS platforms 3. Backup Tracking View (pg_stat_backups) Proposal: Track backup details, including the backup type, status, and user initiating the backup. Proposed fields could include: • backup_type: Type of backup (e.g., pg_adump, pg_dumpall, pg-basebackup, physical, logical) • backup_status: Current backup status (e.g., Running, Completed, Failed) • backup_details: Error messages or backup duration • command: The command used to initiate the backup • user: The user who initiated the backup • client_address: Client IP • client_application: Client application used to perform the backup • backup_start_time, backup_end_time: Timestamps for the backup lifecycle Benefits: • Full visibility into backup processes and their status • Tracks who performed the backup and from which application • Offers a detailed audit trail and error reporting for troubleshooting backup failures • Enables proactive backup monitoring, especially in managed environments 4. System Metadata Proposal: Introduce a system metadata view that exposes key system performance data (e.g., CPU usage, memory, disk space) and OS details, particularly useful for cloud-based PostgreSQL instances. A query might look like: Suggested Columns: • hostname: Hostname of the server • server_ip: IP address of the server • os_version: Operating system version • cpu_model: CPU model and architecture • cpu_cores: Number of CPU cores • RAM: Total available memory • os_uptime: OS uptime since the last reboot Benefits: • Provides critical system performance data without requiring OS-level access • Useful for DBaaS environments where direct server access is not available • Simplifies remote system monitoring and diagnostics 5. Log and WAL Directory Path Exposure Proposal: Enhance PostgreSQL to expose the full paths of log and WAL directories via SQL commands (e.g., data_directory_path). This would improve transparency and troubleshooting in cloud environments where file system access is typically restricted. Benefits: • Transparency: Easy access to the actual file paths of log and WAL directories • Troubleshooting: Facilitates log management and helps pinpoint issues with file access • Ideal for cloud environments where filesystem access is limited 6. Parameter Change Tracking Proposal: Introduce a mechanism to track changes to PostgreSQL configuration parameters, logging who made the changes, the previous values, and the timestamp of the last change. This could look like: Suggested Fields: • previous_value: The value of the parameter before the change • changed_by: The user who made the change • change_time: Timestamp of the change Benefits: • Provides a detailed audit trail of configuration changes • Helps with troubleshooting issues caused by parameter modifications • Enhances accountability in cloud-managed environments Conclusion These proposed features aim to improve PostgreSQL’s functionality in cloud environments, where administrators typically lack direct OS access to the underlying systems. By adding SQL-based management features for configuration, login monitoring, backup tracking, and system performanc
Feature: psql - display current search_path in prompt
Hi all, I need to switch search_paths often. It would be tremendously helpful to see the current search_path in the prompt. - Lauri Siltanen
Re: Feature: psql - display current search_path in prompt
On Sat, 7 Jun 2025 at 20:52, Lauri Siltanen wrote: > I need to switch search_paths often. It would be tremendously helpful to see > the current search_path in the prompt. That feature should be pretty easy to implement, now that search_path is marked as GUC_REPORT in PG18. Basically you need to use PQparameterStatus like we do for session_authorization[1][2]. [1]: https://github.com/postgres/postgres/blob/73e26cbeb5927053eea4e209e5eda34a30c353f1/src/bin/psql/prompt.c#L166-L169 [2]: https://github.com/postgres/postgres/blob/73e26cbeb5927053eea4e209e5eda34a30c353f1/src/bin/psql/common.c#L2508-L2520
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
On Sun, 8 Jun 2025 at 01:35, Robert Treat wrote: > > On Fri, Jun 6, 2025 at 8:04 PM David Rowley wrote: > > Can you list your proposed series of steps you'd recommend to a DBA > > wishing to remove an index, assuming this feature exists in core as > > you'd like it to? > > > > Well, the series of steps differs depending on the nature of the > system being managed. If you are running on a single node with normal > traffic and resources, you just set the GUC to include the index you > want to be invisible, wait for a few days (maybe no one runs monthly > reports on this system?), take a quick look at your monitoring/stats > to make sure things seem copacetic, and then you drop the index and > reset the GUC. Thanks for explaining. What are your thoughts on cached plans? In this scenario, do you assume that waiting a few days means that connections get reset and prepared statements will have been replanned? Or do you think cached plans don't matter in this scenario? David
Re: Enhancing PostgreSQL Management and Observability in Cloud Environments
On Fri, 2025-06-06 at 09:08 +0530, sreekanta reddy 1996 wrote: > I am writing to propose a set of features aimed at significantly enhancing > PostgreSQL’s management and observability in cloud environments, particularly > for administrators who do not have direct access to the underlying operating > system of database servers. These suggestions focus on improving security > auditing, backup tracking, system metrics visibility, and configuration > management—all through SQL-level commands. In a cloud environment, you don't get a superuser, so in looking at the following we have to keep that in mind. > 1. Dynamic pg_hba.conf Management > Proposal: > Introduce SQL-based commands to dynamically manage pg_hba.conf entries, > removing the > need for manual file edits. For example: > ALTER SYSTEM ADD PG_HBA ( > type => 'host', > database => 'mydb', > user => 'myuser', > address => '192.168.1.0/24', > method => 'md5', > comment => 'This connection from app_prod' > ); > Benefits: > • Enables safe, cloud-based management of the pg_hba.conf file > • Supports dynamic configuration without requiring direct OS access > • Ideal for DBaaS environments where administrators may not have OS-level > privileges If you want to allow that to a non-superuser, you'd have to invent a new role whose members are allowed to do that. But looking at the example of "postgresql.conf", it might also make sense to invent something like ALTER SYSTEM for superusers to ease maintenance. With ALTER SYSTEM it was decided to invent a second configuration file (postgresql.auto.conf), but I fail to see how that could work with "pg_hba.conf", aince order matters a lot in that file. So the command would have to edit the existing "pg_hba.conf". That would be tricky if the file contains any relevant comments. Also, you'd have to think of a way to specify where exactly the new line is to be added. All in all, I consider this difficult to implement in a useful fashion. > 2. Login Monitoring > Proposal: > Tracks login attempts and offers key insights into authentication status. > Key fields might include: > • user: Database user attempting to login > • db_name: Database name > • client_address: IP address of the client > • client_application: The application from which the connection originated > • last_attempt_time: Timestamp of the last login attempt > • last_attempt_status: Outcome of the last login attempt (e.g., success, > failed) > • connection_status_remarks: Detailed remarks on the connection (e.g., no > match in pg_hba.conf, SSL-related errors, etc.) > Benefits: > • Facilitates detailed security auditing of login attempts > • Helps troubleshoot authentication problems > • Provides insights into connection patterns and potential issues > • Enhances monitoring and security in DBaaS platforms There is already "log_connections". Doesn't that do all of the above, if you set an appropriate "log_line_prefix"? > 3. Backup Tracking View (pg_stat_backups) Proposal: Track backup details, > including the > backup type, status, and user initiating the backup. Proposed fields > could include: > • backup_type: Type of backup (e.g., pg_adump, pg_dumpall, pg-basebackup, > physical, logical) > • backup_status: Current backup status (e.g., Running, Completed, Failed) > • backup_details: Error messages or backup duration > • command: The command used to initiate the backup > • user: The user who initiated the backup > • client_address: Client IP > • client_application: Client application used to perform the backup > • backup_start_time, backup_end_time: Timestamps for the backup lifecycle > Benefits: > • Full visibility into backup processes and their status > • Tracks who performed the backup and from which application > • Offers a detailed audit trail and error reporting for troubleshooting > backup failures > • Enables proactive backup monitoring, especially in managed environments I wouldn't mix pg_dump and file system backup. People run pg_dump for all kings of reasons other than backup, so tracking that would be confusing. Backups are already tracked in the *.backup files in the archive, but I guess you want something accessible from SQL. That might be feasible, but I see the problem of backups expiring: At some point, you want to get rid of old backups, and that happens outside the database, so your tracking view would get out of sync with reality. And then it would be much less useful. > 4. System Metadata > Proposal: > Introduce a system metadata view that exposes key system performance data > (e.g., > CPU usage, memory, disk space) and OS details, particularly useful for > cloud-based > PostgreSQL instances. A query might look like: > Suggested Columns: > • hostname: Hostname of the server > • server_ip: IP address of the server > • os_version: Operating system version > • cpu_model: CPU model and architecture
Re: Sanding down some edge cases for PL/pgSQL reserved words
Hi I started reviewing this patch. so 7. 6. 2025 v 18:41 odesílatel Tom Lane napsal: > This is a rather delayed response to the discussion of bug > #18693 [1], in which I wrote: > > > (It's kind of annoying that "strict" has to be double-quoted > > in the RAISE NOTICE, especially since you get a rather misleading > > error if it isn't. But that seems like a different discussion.) > > As an example of that, if you don't double-quote "strict" > in this usage you get > > regression=# do $$ declare r record; begin > SELECT a, b AS STRICT INTO r FROM (SELECT 'A' AS a, 'B' AS b) AS q; > RAISE NOTICE 'STRICT r.strict = %', r.strict; > end $$; > ERROR: record "r" has no field "strict" > LINE 1: r.strict > ^ > QUERY: r.strict > CONTEXT: PL/pgSQL function inline_code_block line 3 at RAISE > > which is pretty bogus because the record *does* have a field > named "strict". The actual problem is that STRICT is a fully > reserved PL/pgSQL keyword, which means you need to double-quote > it if you want to use it this way. > > The attached patches provide two independent responses to that: > > 1. AFAICS, there is no real reason for STRICT to be a reserved > rather than unreserved PL/pgSQL keyword, and for that matter not > EXECUTE either. Making them unreserved does allow some ambiguity, > but I don't think there's any surprises in how that ambiguity > would be resolved; and certainly we've preferred ambiguity over > introducing new reserved keywords in PL/pgSQL before. I think > these two just escaped that treatment by dint of being ancient. > There is no issue. > > 2. That "has no field" error message is flat-out wrong. The now-known > way to trigger it has a different cause, and what's more, we simply do > not know at this point whether the malleable record type has such a > field. So in 0002 below I just changed it to assume that the problem > is a reserved field name. We might find another way to reach that > failure in future, but I doubt that "has no field" would be the right > thing to say in any case. > The proposed patch is a zero invasive solution. But the question is why we cannot allow plpgsql reserved keywords in recfilds? There should not be any collisions. Isn't there a better solution to modify plpgsql_yylex instead and allow all keywords after '.' ? Sure. It will be more invasive. Regards Pavel > This is v19 material at this point, so I'll stick it on the CF queue. > > regards, tom lane > > [1] > https://www.postgresql.org/message-id/flat/18693-65968418890877b4%40postgresql.org > >
