Re: psql: Add role's membership options to the \du+ command
On 14.04.2023 10:28, Kyotaro Horiguchi wrote: As David-G appears to express concern in upthread, I don't think a direct Japanese translation from "rolename from rolename" works well, as the "from" needs accompanying verb. I, as a Japanese speaker, I prefer a more non-sentence-like notation, similar to David's suggestion but with slight differences: "pg_read_all_stats (grantor: postgres, inherit, set)" In this form, it confuses me that 'postgres' and 'inherit, set' look like a common list. Come to think of this, I recalled a past discussion in which we concluded that translating punctuation marks appearing between a variable number of items within list expressions should be avoided... Thus, I'd like to propose to use an ACL-like notation, which doesn't need translation. ..| Mamber of | ..| pg_read_server_files=ais/horiguti,pg_execute_server_program=/postgres | It's very tempting to do so. But I don't like this approach. Showing membership options as an ACL-like column will be confusing. Even in your example, my first reaction is that pg_execute_server_program is available to public. (As for the general patterns, we can also consider combining three options into one column, like pg_class.reloptions.) So, yet another way to discuss: pg_read_all_stats(inherit,set/horiguti) pg_execute_server_program(empty/postgres) One more point. Grants without any option probably should be prohibited as useless. But this is for a new thread. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Re: [RFC] Add jit deform_counter
> On Fri, Mar 31, 2023 at 07:39:27PM +0200, Dmitry Dolgov wrote:
> > On Wed, Mar 29, 2023 at 01:50:37PM +1300, David Rowley wrote:
> > On Sun, 12 Jun 2022 at 21:14, Dmitry Dolgov <9erthali...@gmail.com> wrote:
> > > I've noticed that JIT performance counter generation_counter seems to
> > > include
> > > actions, relevant for both jit_expressions and jit_tuple_deforming
> > > options. It
> > > means one can't directly see what is the influence of jit_tuple_deforming
> > > alone, which would be helpful when adjusting JIT options. To make it
> > > better a
> > > new counter can be introduced, does it make sense?
> >
> > I'm not so sure about this idea. As of now, if I look at EXPLAIN
> > ANALYZE's JIT summary, the individual times add up to the total time.
> >
> > If we add this deform time, then that's no longer going to be true as
> > the "Generation" time includes the newly added deform time.
> >
> > master:
> > JIT:
> >Functions: 600
> >Options: Inlining false, Optimization false, Expressions true, Deforming
> > true
> >Timing: Generation 37.758 ms, Inlining 0.000 ms, Optimization 6.736
> > ms, Emission 172.244 ms, Total 216.738 ms
> >
> > 37.758 + 6.736 + 172.244 = 216.738
> >
> > I think if I was a DBA wondering why JIT was taking so long, I'd
> > probably either be very astonished or I'd report a bug if I noticed
> > that all the individual component JIT times didn't add up to the total
> > time.
> >
> > I don't think the solution is to subtract the deform time from the
> > generation time either.
> >
> > Can't users just get this by looking at EXPLAIN ANALYZE with and
> > without jit_tuple_deforming?
>
> It could be done this way, but then users need to know that tuple
> deforming is included into generation time (I've skimmed through the
> docs, there seems to be no direct statements about that, although it
> could be guessed). At the same time I don't think it's very
> user-friendly approach -- after all it could be the same for other
> timings, i.e. only one counter for all JIT operations present,
> expecting users to experiment how would it change if this or that option
> will be different.
>
> I agree about adding up to the total time though. What about changing
> the format to something like this?
>
>Options: Inlining false, Optimization false, Expressions true, Deforming
> true
>Timing: Generation 37.758 ms (Deforming 1.234 ms), Inlining 0.000 ms,
> Optimization 6.736 ms, Emission 172.244 ms, Total 216.738 ms
>
> This way it doesn't look like deforming timing is in the same category
> as others, but rather a part of another value.
Here is the patch with the proposed variation.
>From 963b9a31f2241cfff766544685709d813145f33a Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 11 Jun 2022 11:54:40 +0200
Subject: [PATCH v5 1/2] Add deform_counter
At the moment generation_counter includes time spent on both JITing
expressions and tuple deforming. Those are configured independently via
corresponding options (jit_expressions and jit_tuple_deforming), which
means there is no way to find out what fraction of time tuple deforming
alone is taking.
Add deform_counter dedicated to tuple deforming, which allows seeing
more directly the influence jit_tuple_deforming is having on the query.
---
src/backend/commands/explain.c | 7 ++-
src/backend/jit/jit.c | 1 +
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/include/jit/jit.h | 3 +++
4 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 5334c503e1..a134411209 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -893,6 +893,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
/* calculate total time */
INSTR_TIME_SET_ZERO(total_time);
+ /* don't add deform_counter, it's included into generation_counter */
INSTR_TIME_ADD(total_time, ji->generation_counter);
INSTR_TIME_ADD(total_time, ji->inlining_counter);
INSTR_TIME_ADD(total_time, ji->optimization_counter);
@@ -920,8 +921,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
{
ExplainIndentText(es);
appendStringInfo(es->str,
- "Timing: %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
+ "Timing: %s %.3f ms (%s %.3f ms), %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
"Generation", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->generation_counter),
+ "Deforming", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->deform_counter),
"Inlining", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->inlining_counter),
"Optimization", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->optimization_counter),
"Emission", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->emission_counter),
@@ -948,6 +950,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
ExplainPropertyFloat("Generation", "ms",
1000.0 *
Re: Protecting allocator headers with Valgrind
On Wed, Apr 12, 2023 at 01:28:08AM +1200, David Rowley wrote: > Any objections? Not objecting. I think the original Valgrind integration refrained from this because it would have added enough Valgrind client requests to greatly slow Valgrind runs. Valgrind reduced the cost of client requests in later years, so this new conclusion is reasonable.
Incremental sort for access method with ordered scan support (amcanorderbyop)
Current version of postgresql don't support incremental sort using
ordered scan on access method.
Example database:
CREATE TABLE t (id serial, p point, PRIMARY KEY(id));
INSERT INTO t (SELECT generate_series(1, 1000), point(random(), random()));
CREATE INDEX p_idx ON t USING gist(p);
ANALYZE;
Now i want closest points to center:
SELECT id, p <-> point(0.5, 0.5) dist FROM t ORDER BY dist LIMIT 10;
Everything works good (Execution Time: 0.276 ms).
Now i want predictable sorting for points with same distance:
SELECT id, p <-> point(0.5, 0.5) dist FROM t ORDER BY dist, id LIMIT 10;
Execution time is now 1 000 x slower (589.486 ms) and postgresql uses
full sort istead of incremental:
Sort (cost=205818.51..216235.18 rows=417 width=12)
Postgres allows incremental sort only for ordered indexes. Function
build_index_paths dont build partial order paths for access methods
with order support. My patch adds support for incremental ordering
with access method. Results with patch:
Incremental Sort (cost=5522.10..1241841.02 rows=1000 width=12)
(actual time=0.404..0.405 rows=10 loops=1)
Sort Key: ((p <-> '(0.5,0.5)'::point)), id
Presorted Key: ((p <-> '(0.5,0.5)'::point))
Execution Time: 0.437 ms
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 9f4698f2a2..c580d657cf 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -186,7 +186,8 @@ static IndexClause *expand_indexqual_rowcompare(PlannerInfo *root,
bool var_on_left);
static void match_pathkeys_to_index(IndexOptInfo *index, List *pathkeys,
List **orderby_clauses_p,
- List **clause_columns_p);
+ List **clause_columns_p,
+ int *match_pathkeys_length_p);
static Expr *match_clause_to_ordering_op(IndexOptInfo *index,
int indexcol, Expr *clause, Oid pk_opfamily);
static bool ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel,
@@ -853,6 +854,7 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
bool index_is_ordered;
bool index_only_scan;
int indexcol;
+ int match_pathkeys_length;
/*
* Check that index supports the desired scan type(s)
@@ -977,9 +979,14 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
/* see if we can generate ordering operators for query_pathkeys */
match_pathkeys_to_index(index, root->query_pathkeys,
&orderbyclauses,
-&orderbyclausecols);
- if (orderbyclauses)
- useful_pathkeys = root->query_pathkeys;
+&orderbyclausecols,
+&match_pathkeys_length);
+ if (orderbyclauses) {
+ if (match_pathkeys_length == list_length(root->query_pathkeys))
+useful_pathkeys = root->query_pathkeys;
+ else
+useful_pathkeys = list_truncate(list_copy(root->query_pathkeys), match_pathkeys_length);
+ }
else
useful_pathkeys = NIL;
}
@@ -3065,7 +3072,8 @@ expand_indexqual_rowcompare(PlannerInfo *root,
static void
match_pathkeys_to_index(IndexOptInfo *index, List *pathkeys,
List **orderby_clauses_p,
- List **clause_columns_p)
+ List **clause_columns_p,
+ int *match_pathkeys_length_p)
{
List *orderby_clauses = NIL;
List *clause_columns = NIL;
@@ -3073,6 +3081,7 @@ match_pathkeys_to_index(IndexOptInfo *index, List *pathkeys,
*orderby_clauses_p = NIL; /* set default results */
*clause_columns_p = NIL;
+ *match_pathkeys_length_p = 0;
/* Only indexes with the amcanorderbyop property are interesting here */
if (!index->amcanorderbyop)
@@ -3092,11 +3101,11 @@ match_pathkeys_to_index(IndexOptInfo *index, List *pathkeys,
/* Pathkey must request default sort order for the target opfamily */
if (pathkey->pk_strategy != BTLessStrategyNumber ||
pathkey->pk_nulls_first)
- return;
+ break;
/* If eclass is volatile, no hope of using an indexscan */
if (pathkey->pk_eclass->ec_has_volatile)
- return;
+ break;
/*
* Try to match eclass member expression(s) to index. Note that child
@@ -3140,12 +3149,14 @@ match_pathkeys_to_index(IndexOptInfo *index, List *pathkeys,
}
}
- if (found) /* don't want to look at remaining members */
+ if (found) { /* don't want to look at remaining members */
+(*match_pathkeys_length_p)++;
break;
+ }
}
if (!found)/* fail if no match for this pathkey */
- return;
+ break;
}
*orderby_clauses_p = orderby_clauses; /* success! */
Re: Unexpected (wrong?) result querying boolean partitioned table with NULL partition
On Wed, Apr 12, 2023 at 4:13 AM David Rowley wrote: > > There seems to be a bunch of tests checking this already, all of them > assuming the incorrect plans. Given that the plan alone wasn't sufficient to catch this error previously, would it be worthwhile to add some data to the tests to make it abundantly obvious? I had noticed that the default partition seems to be an edge case in the code. Perhaps it's overkill, but would it be worth adding a test where the NULL partition is not the default? Thanks, David
Re: Direct I/O
On 2023-04-14 21:33, Andres Freund wrote: Oh! I was misled by the buildfarm label on morepork, which claims it's running clang 10.0.1. But actually, per its configure report, it's running configure: using compiler=gcc (GCC) 4.2.1 20070719 Huh. I wonder if that was an accident in the BF setup. I might have been when I reinstalled it a while ago. I have the following gcc and clang installed: openbsd_6_9-pgbf$ gcc --version gcc (GCC) 4.2.1 20070719 Copyright (C) 2007 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. openbsd_6_9-pgbf$ clang --version OpenBSD clang version 10.0.1 Target: amd64-unknown-openbsd6.9 Thread model: posix InstalledDir: /usr/bin want me to switch to clang instead? /Mikael
Re: Direct I/O
Thomas Munro writes: > As far as I can tell, the failure to honour large alignment attributes > even though the compiler passes our configure check that you can do > that was considered to be approximately a bug[1] or at least a thing > to be improved in fairly old GCC times but the fix wasn't back-patched > that far. Unfortunately the projects that were allergic to the GPL3 > change but wanted to ship a compiler (or some motivation related to > that) got stuck on 4.2 for a while before they flipped to Clang (as > OpenBSD has now done). It seems hard to get excited about doing > anything about that on our side, and those systems are also spewing > other warnings. But if we're going to do it, it looks like the right > place would indeed be a new compiler check that the attribute exists > *and* generates no warnings with alignment > 16, something like that? I tested this by building gcc 4.2.1 from source on modern Linux (which was a bit more painful than it ought to be, perhaps) and building PG with that. It generates no warnings, but nonetheless gets an exception in CREATE DATABASE: #2 0x00b64522 in ExceptionalCondition ( conditionName=0xd4fca0 "(uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, buffer)", fileName=0xd4fbe0 "md.c", lineNumber=468) at assert.c:66 #3 0x009a6b53 in mdextend (reln=0x1dcaf68, forknum=MAIN_FORKNUM, blocknum=18, buffer=0x7ffcaf8e1af0, skipFsync=true) at md.c:468 #4 0x009a9075 in smgrextend (reln=0x1dcaf68, forknum=MAIN_FORKNUM, blocknum=18, buffer=0x7ffcaf8e1af0, skipFsync=true) at smgr.c:500 #5 0x0096739c in RelationCopyStorageUsingBuffer (srclocator=..., dstlocator=..., forkNum=MAIN_FORKNUM, permanent=true) at bufmgr.c:4286 #6 0x00967584 in CreateAndCopyRelationData (src_rlocator=..., dst_rlocator=..., permanent=true) at bufmgr.c:4361 #7 0x0068898e in CreateDatabaseUsingWalLog (src_dboid=1, dst_dboid=24576, src_tsid=1663, dst_tsid=1663) at dbcommands.c:217 #8 0x0068b594 in createdb (pstate=0x1d4a6a8, stmt=0x1d20ec8) at dbcommands.c:1441 Sure enough, that buffer is a stack local in RelationCopyStorageUsingBuffer, and it's visibly got a not-very-well-aligned address. So apparently, the fact that you even get a warning about the alignment not being honored is something OpenBSD patched in after-the-fact; it's not there in genuine vintage gcc. I get the impression that we are going to need an actual runtime test if we want to defend against this. Not entirely convinced it's worth the trouble. Who, other than our deliberately rear-guard buildfarm animals, is going to be building modern PG with such old compilers? (And more especially to the point, on platforms new enough to have working O_DIRECT?) At this point I agree with Andres that it'd be good enough to silence the warning by getting rid of these alignment pragmas when the platform lacks O_DIRECT. regards, tom lane PS: I don't quite understand how it managed to get through initdb when CREATE DATABASE doesn't work. Maybe there is a different code path taken in standalone mode?
Race conditions in has_table_privilege() and friends
I noticed that prion recently failed [1] with
SELECT schema_to_xml_and_xmlschema('testxmlschema', true, true, 'foo');
...
+ERROR: relation with OID 30019 does not exist
+CONTEXT: SQL statement "SELECT oid FROM pg_catalog.pg_class WHERE
relnamespace = 29949 AND relkind IN ('r','m','v') AND
pg_catalog.has_table_privilege (oid, 'SELECT') ORDER BY relname;"
What evidently happened here is that has_table_privilege got applied
to some relation (in a schema different from 'testxmlschema', which
should be stable here) that was in the middle of getting dropped.
This'd require the relnamespace condition to get applied after the
has_table_privilege condition, which is quite possible (although it seems
to require that auto-analyze update pg_class's statistics while this
test runs). Even then, has_table_privilege is supposed to survive the
situation, but it has a race condition:
if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(tableoid)))
PG_RETURN_NULL();
aclresult = pg_class_aclcheck(tableoid, roleid, mode);
The fact that SearchSysCacheExists1 succeeds doesn't guarantee that
when pg_class_aclcheck fetches the same row a moment later, it'll
still be there. The race-condition window is pretty narrow (and
indeed I don't think we've seen this buildfarm symptom before),
but it exists.
Now, it looks to me like this case is trivial to fix, using the
pg_class_aclcheck_ext function introduced in b12bd4869 (in v14).
We just need to drop the separate SearchSysCacheExists1 call and do
aclresult = pg_class_aclcheck_ext(tableoid, roleid, mode, &is_missing);
which should be a trifle faster as well as safer.
However, to cover the remaining risk spots in acl.c, it looks like
we'd need "_ext" versions of pg_attribute_aclcheck_all and
object_aclcheck, which were not introduced by b12bd4869.
object_aclcheck_ext in particular looks like it'd be a bit invasive.
What I'm inclined to do for now is clean up the table-related cases
and leave the code paths using object_aclcheck for another day.
We've always been much more concerned about DDL race conditions for
tables than other kinds of objects, so this approach seems to fit
with past decisions. I haven't written any code yet, but this looks
like it might amount to a couple hundred lines of fairly simple
changes.
I wonder if we should consider this a bug and back-patch to v14,
or maybe HEAD only; or is it okay to let it slide to v17?
regards, tom lane
[1]
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2023-04-15%2017%3A17%3A09
Re: Should vacuum process config file reload more often
> On 11 Apr 2023, at 17:05, Masahiko Sawada wrote: > The comment of message_level_is_interesting() says: > > * This is useful to short-circuit any expensive preparatory work that > * might be needed for a logging message. > > Which can apply to taking a lwlock, I think. I agree that we can, and should, use message_level_is_interesting to skip taking this lock. Also, the more I think about the more I'm convinced that we should not change the current logging frequency of once per table from what we ship today. In DEGUG2 the logs should tell the whole story without requiring extrapolation based on missing entries. So I think we should use your patch to solve this open item. If there is interest in reducing the logging frequency we should discuss that in its own thread, insted of it being hidden in here. -- Daniel Gustafsson
Re: segfault tied to "IS JSON predicate" commit
On Thu, Apr 13, 2023 at 09:14:01PM -0700, Peter Geoghegan wrote:
> I find that if I run the following test against a standard debug build
> on HEAD, my local installation reliably segfaults:
>
> $ meson test --setup running --suite test_rls_hooks-running
>
> Attached is a "bt full" run from gdb against a core dump. The query
> "EXPLAIN (costs off) SELECT * FROM rls_test_permissive;" runs when the
> backend segfaults.
>
> The top frame of the back trace is suggestive of a use-after-free:
>
> #0 copyObjectImpl (from=0x7f7f7f7f7f7f7f7e) at copyfuncs.c:187
> 187 switch (nodeTag(from))
> ...
>
> "git bisect" suggests that the problem began at commit 6ee30209,
> "SQL/JSON: support the IS JSON predicate".
>
> It's a bit surprising that the bug reproduces when I run a standard
> test, and yet we appear to have a bug that's about 2 weeks old. There
> may be something unusual about my system that will turn out to be
> relevant -- though there is nothing particularly exotic about this
> machine. My repro doesn't rely on concurrent execution, or timing, or
> anything like that -- it's quite reliable.
I was able to reproduce this yesterday but not today.
I think what happened is that you (and I) are in the habbit of running
"meson test tmp_install" to compile new binaries and install them into
./tmp_install, and then run a server out from there. But nowadays
there's also "meson test install_test_files". I'm not sure what
combination of things are out of sync, but I suspect you forgot one of
0) compile *and* install the new binaries; or 1) restart the running
postmaster; or, 2) install the new shared library ("test files").
I saw the crash again when I did this:
time ninja
time meson test tmp_install install_test_files regress/regress # does not
recompile, BTW
./tmp_install/usr/local/pgsql/bin/postgres -D
./testrun/regress/regress/tmp_check/data -p 5678 -c autovacuum=no&
git checkout HEAD~222
time meson test tmp_install install_test_files
time PGPORT=5678 meson test --setup running test_rls_hooks-running/regress
In this case, I'm not sure if there's anything to blame meson for; the
issue is running server, which surely has different structures since
last month.
--
Justin
Re: Direct I/O
On Sun, Apr 16, 2023 at 6:19 AM Tom Lane wrote: > So apparently, the fact that you even get a warning about the > alignment not being honored is something OpenBSD patched in > after-the-fact; it's not there in genuine vintage gcc. Ah, that is an interesting discovery, and indeed kills the configure check idea. > At this point I agree with Andres that it'd be good enough to > silence the warning by getting rid of these alignment pragmas > when the platform lacks O_DIRECT. Hmm. My preferred choice would be: accept Mikael's kind offer to upgrade curculio to a live version, forget about GCC 4.2.1 forever, and do nothing. It is a dead parrot. But if we really want to do something about this, my next preferred option would be to modify c.h's test to add more conditions, here: /* GCC, Sunpro and XLC support aligned, packed and noreturn */ #if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__) #define pg_attribute_aligned(a) __attribute__((aligned(a))) ... Full GCC support including stack objects actually began in 4.6, it seems. It might require a bit of research because the GCC-workalikes including Clang also claim to be certain versions of GCC (for example I think Clang 7 would be excluded if you excluded GCC 4.2, even though this particular thing apparently worked fine in Clang 7). That's my best idea, ie to actually model the feature history accurately, if we are suspending disbelief and pretending that it is a reasonable target.
Re: Direct I/O
Thomas Munro writes: > Full GCC support including stack objects actually began in 4.6, it > seems. Hmm. The oldest gcc versions remaining in the buildfarm seem to be curculio | configure: using compiler=gcc (GCC) 4.2.1 20070719 frogfish | configure: using compiler=gcc (Debian 4.6.3-14) 4.6.3 lapwing | configure: using compiler=gcc (Debian 4.7.2-5) 4.7.2 skate | configure: using compiler=gcc-4.7 (Debian 4.7.2-5) 4.7.2 snapper | configure: using compiler=gcc-4.7 (Debian 4.7.2-5) 4.7.2 buri | configure: using compiler=gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44) chub | configure: using compiler=gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44) dhole | configure: using compiler=gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44) mantid| configure: using compiler=gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44) prion | configure: using compiler=gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-28) rhinoceros| configure: using compiler=gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44) siskin| configure: using compiler=gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44) shelduck | configure: using compiler=gcc (SUSE Linux) 4.8.5 topminnow | configure: using compiler=gcc (Debian 4.9.2-10+deb8u1) 4.9.2 so curculio should be the only one that's at risk here. Maybe just upgrading it is the right answer. regards, tom lane
Re: Direct I/O
On Sat, Apr 15, 2023 at 02:19:35PM -0400, Tom Lane wrote: > PS: I don't quite understand how it managed to get through initdb > when CREATE DATABASE doesn't work. Maybe there is a different > code path taken in standalone mode? ad43a413c4f7f5d024a5b2f51e00d280a22f1874 initdb: When running CREATE DATABASE, use STRATEGY = WAL_COPY.
Re: segfault tied to "IS JSON predicate" commit
On Sat, Apr 15, 2023 at 2:46 PM Justin Pryzby wrote: > I think what happened is that you (and I) are in the habbit of running > "meson test tmp_install" to compile new binaries and install them into > ./tmp_install, and then run a server out from there. That's not my habit; this is running against a server that was installed into a dedicated install directory. Though I agree that an issue with the environment seems likely. > But nowadays > there's also "meson test install_test_files". That only applies with "--setup tmp_install", which is the default test setup, and the one that you must be using implicitly. But I'm using "--setup running" for this. More concretely, the tmp_install setup has the tests you say are requirements: $ meson test --setup tmp_install --list | grep install postgresql:setup / tmp_install postgresql:setup / install_test_files But not the running setup: $ meson test --setup running --list | grep install | wc -l 0 I'm aware of the requirement around specifying "--suite tmp_install ..." right before "... --suite what_i_really_want_to_test" is specified. However, I can't see how it could be my fault for forgetting that, since it's structurally impossible to specify "--suite tmp_install" when using "--setup running". I was using the setup that gives you behavior that's approximately equivalent to "make installcheck" (namely "--setup running"), so naturally this would have been impossible. Let's review: * There are two possible --setup modes. I didn't use the default (which is "--setup tmp_install") here. Rather, I used "--setup running", which is kinda like "make installcheck". * There is a test suite named "setup", though it's only available with "--setup tmp_install", the default setup. (This is not to be confused with the meson-specific notion of a --setup.) * The "setup" suite happens to contain an individual test called "tmp_install" (as well as one called "install_test_files") * I cannot possibly have forgotten this, since asking for it with "--setup running" just doesn't work. Let's demonstrate what I mean. The following does not and cannot work, so I cannot have forgotten to do it in any practical sense: $ meson test --setup running postgresql:setup / tmp_install ninja: no work to do. No suitable tests defined. Such an incantation can only be expected to work with --setup tmp_install, the default. So this version does work: $ meson test --setup tmp_install postgresql:setup / tmp_install **SNIP** 1/1 postgresql:setup / tmp_installOK 0.72s **SNIP** Not confusing at all! -- Peter Geoghegan
Re: v16dev: invalid memory alloc request size 8488348128
On Sat, Apr 15, 2023 at 11:33:58AM +1200, David Rowley wrote:
> On Sat, 15 Apr 2023 at 10:48, Justin Pryzby wrote:
> >
> > On Sat, Apr 15, 2023 at 10:04:52AM +1200, David Rowley wrote:
> > > Which aggregate function is being called here? Is it a custom
> > > aggregate written in C, by any chance?
> >
> > That function is not an aggregate:
>
> There's an aggregate somewhere as indicated by this fragment from the
> stack trace:
>
> > #12 project_aggregates (aggstate=aggstate@entry=0x607200070d38) at
> > ../src/backend/executor/nodeAgg.c:1377
> > #13 0x00903eb6 in agg_retrieve_direct
> > (aggstate=aggstate@entry=0x607200070d38) at
> > ../src/backend/executor/nodeAgg.c:2520
> > #14 0x00904074 in ExecAgg (pstate=0x607200070d38) at
> > ../src/backend/executor/nodeAgg.c:2172
>
> Any chance you could try and come up with a minimal reproducer? You
> have access to see which aggregates are being used here and what data
> types are being given to them and then what's being done with the
> return value of that aggregate that's causing the crash. Maybe you
> can still get the crash if you mock up some data to aggregate and
> strip out the guts from the plpgsql functions that we're crashing on?
Try this
CREATE OR REPLACE FUNCTION avfinal(x anycompatiblearray) RETURNS
anycompatiblearray
LANGUAGE plpgsql AS $$
DECLARE
res real[];
BEGIN
res := ARRAY_FILL(x[1], ARRAY[4]);
RETURN res;
END;
$$;
CREATE OR REPLACE FUNCTION acc(x anycompatiblearray, y anycompatiblearray)
RETURNS anycompatiblearray
LANGUAGE plpgsql AS $$
BEGIN
RETURN ARRAY_FILL(y[1], ARRAY[4]);
END;
$$;
CREATE OR REPLACE AGGREGATE public.av(anycompatiblearray) (
STYPE = anycompatiblearray,
INITCOND = '{{0},{0}}',
SFUNC = acc,
FINALFUNC = avfinal
);
CREATE OR REPLACE FUNCTION aw(real[])
RETURNS void LANGUAGE plpgsql
AS $function$
begin
end
$function$;
SELECT
aw(av(ARRAY[1.0::real])),
aw(av(ARRAY[1.0::real])),
1;
Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
On Sat, 15 Apr 2023 at 12:59, David Rowley wrote: > These are all valid points. I've attached a patch aiming to address > each of them. I tweaked this a little further and pushed it. David
Re: segfault tied to "IS JSON predicate" commit
On Sat, Apr 15, 2023 at 4:11 PM Peter Geoghegan wrote:
> $ meson test --setup tmp_install --list | grep install
> postgresql:setup / tmp_install
> postgresql:setup / install_test_files
>
> But not the running setup:
>
> $ meson test --setup running --list | grep install | wc -l
> 0
There is a concrete problem here: commit b6a0d469ca ("meson: Prevent
installation of test files during main install") overlooked "--setup
running". It did not add a way for the setup to run "postgresql:setup
/ install_test_files" (or perhaps something very similar).
The segfault must have been caused by unwitting use of a leftover
ancient test_rls_hooks.so from before commit b6a0d469ca. My old stale
.so must have continued to work for a little while, before it broke.
Now that I've fully deleted my install directory, I can see a clear
problem, which is much less mysterious than the segfault. Namely, the
following doesn't still work:
$ meson test --setup running --suite test_rls_hooks-running
This time it's not a segfault, though -- it's due to the .so being
unavailable. Adding "--suite setup" fixes nothing, since I'm using
"--setup running"; while the "--suite running" tests will actually run
and install the .so, they won't install it into the installation
directory I'm actually using (only into a tmp_install directory).
While I was wrong to implicate commit 6ee30209 (the IS JSON commit) at
first, there is a bug here. A bug in b6a0d469ca.
ISTM that b6a0d469ca has created an unmet need for a "--suite
setup-running", which is analogous to "--suite setup" but works with
"--setup running". That way there'd at least be a
"postgresql:setup-running / install_test_files" test that could be
used here, like so:
$ meson test --setup running --suite setup-running --suite
test_rls_hooks-running
But...maybe it would be better to find a way to install the stuff from
"postgresql:setup / install_test_files" in a less kludgy, more
standard kind of way? I see that the commit message from b6a0d469ca
says "there is no way to set up up the build system to install extra
things only when told". Is that *really* the case?
--
Peter Geoghegan
Re: Where are we on supporting LLVM's opaque-pointer changes?
On Sat, Apr 15, 2023 at 2:31 AM Tom Lane wrote: > I know we've been letting this topic slide, but we are out of runway. > I propose adding this as a must-fix open item for PG 16. I had a patch that solved many of the problems[1], but it isn't all the way there and I got stuck. I am going to look at it together with Andres in the next couple of days, get unstuck, and aim to get a patch out this week. More soon. [1] https://github.com/macdice/postgres/tree/llvm15
idea: multiple arguments to_regclass function
Hi I missing some variants of to_regclass to_regclass(schemaname, objectname) to_regclass(catalogname, schemaname, objectname) It can helps with object identification, when I have separated schema and name What do you think about this? Regards Pavel
Re: Protecting allocator headers with Valgrind
On Sun, 16 Apr 2023 at 03:26, Noah Misch wrote: > Not objecting. I think the original Valgrind integration refrained from this > because it would have added enough Valgrind client requests to greatly slow > Valgrind runs. Valgrind reduced the cost of client requests in later years, > so this new conclusion is reasonable. I tested that. It's not much slowdown: time make installcheck Unpatched: real79m36.458s Patched: real81m31.589s I forgot to mention, I pushed the patch yesterday. David
