Re: standby promotion can create unreadable WAL
On Tue, Aug 23, 2022 at 12:06 AM Robert Haas wrote:
> Nothing that uses xlogreader is going to be able to bridge the gap
> between file #4 and file #5. In this case it doesn't matter very much,
> because we immediately write a checkpoint record into file #5, so if
> we crash we won't try to replay file #4 anyway. However, if anything
> did try to look at file #4 it would get confused. Maybe that can
> happen if this is a streaming standby, where we only write an
> end-of-recovery record upon promotion, rather than a checkpoint, or
> maybe if there are cascading standbys someone could try to actually
> use the 00020004 file for something. I'm not sure. But
> unless I'm missing something, that file is bogus, and our only hope of
> not having problems is that perhaps no one will ever look at it.
Yeah, this analysis looks correct to me.
> I think that the cause of this problem is this code right here:
>
> /*
> * Actually, if WAL ended in an incomplete record, skip the parts that
> * made it through and start writing after the portion that persisted.
> * (It's critical to first write an OVERWRITE_CONTRECORD message, which
> * we'll do as soon as we're open for writing new WAL.)
> */
> if (!XLogRecPtrIsInvalid(missingContrecPtr))
> {
> Assert(!XLogRecPtrIsInvalid(abortedRecPtr));
> EndOfLog = missingContrecPtr;
> }
Yeah, this statement as well as another statement that creates the
overwrite contrecord. After changing these two lines the problem is
fixed for me. Although I haven't yet thought of all the scenarios
that whether it is safe in all the cases. I agree that after timeline
changes we are pointing to the end of the last valid record we can
start writing the next record from that point onward. But I think we
should need to think hard that whether it will break any case for
which the overwrite contrecord was actually introduced.
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 7602fc8..3d38613 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5491,7 +5491,7 @@ StartupXLOG(void)
* (It's critical to first write an OVERWRITE_CONTRECORD message, which
* we'll do as soon as we're open for writing new WAL.)
*/
- if (!XLogRecPtrIsInvalid(missingContrecPtr))
+ if (newTLI == endOfRecoveryInfo->lastRecTLI &&
!XLogRecPtrIsInvalid(missingContrecPtr))
{
Assert(!XLogRecPtrIsInvalid(abortedRecPtr));
EndOfLog = missingContrecPtr;
@@ -5589,7 +5589,7 @@ StartupXLOG(void)
LocalSetXLogInsertAllowed();
/* If necessary, write overwrite-contrecord before doing
anything else */
- if (!XLogRecPtrIsInvalid(abortedRecPtr))
+ if (newTLI == endOfRecoveryInfo->lastRecTLI &&
!XLogRecPtrIsInvalid(abortedRecPtr))
{
Assert(!XLogRecPtrIsInvalid(missingContrecPtr));
CreateOverwriteContrecordRecord(abortedRecPtr,
missingContrecPtr, newTLI);
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: [PATCH] Optimize json_lex_string by batching character copying
On Wed, Aug 24, 2022 at 12:15 AM Nathan Bossart wrote: > > On Tue, Aug 23, 2022 at 01:03:03PM +0700, John Naylor wrote: > > On Tue, Aug 23, 2022 at 10:32 AM Nathan Bossart > >> Here's a new version of the patch with the 32-bit changes and calls to > >> lfind() removed. > > > > LGTM overall. My plan is to split out the json piece, adding tests for > > that, and commit the infrastructure for it fairly soon. Possible > > bikeshedding: Functions like vector8_eq() might be misunderstood as > > comparing two vectors, but here we are comparing each lane with a > > scalar. I wonder if vector8_eq_scalar() et al might be more clear. > > Good point. I had used vector32_veq() to denote vector comparison, which > would extend to something like vector8_seq(). But that doesn't seem > descriptive enough. It might be worth considering vector8_contains() or > vector8_has() as well. I don't really have an opinion, but if I had to > pick something, I guess I'd choose vector8_contains(). It seems "scalar" would be a bad choice since it already means (confusingly) operating on the least significant element of a vector. I'm thinking of *_has and *_has_le, matching the already existing in the earlier patch *_has_zero. -- John Naylor EDB: http://www.enterprisedb.com
Re: [PATCH] Expose port->authn_id to extensions and triggers
Michael Paquier writes: > On Tue, Aug 23, 2022 at 10:04:30AM -0700, Jacob Champion wrote: >> When can we rely on static initialization, and when can't we? Is there a >> concern that the memory could have been polluted from before the >> postmaster's fork? > My main worry here is EXEC_BACKEND, where we would just use our own > implementation of fork(), and it is a bad idea at the end to leave > that untouched while we could have code paths that attempt to access > it. Uh ... what? EXEC_BACKEND is even more certain to correctly initialize static/global variables in a child process. I agree with Jacob that this memset is probably useless, and therefore confusing. regards, tom lane
Re: ICU for global collation
On Tue, Aug 23, 2022 at 08:59:02PM +0300, Marina Polyakova wrote: > My colleague Andrew Bille found another bug in master > (b4e936859dc441102eb0b6fb7a104f3948c90490) and REL_15_STABLE > (2c63b0930aee1bb5c265fad4a65c9d0b62b1f9da): pg_collation.colliculocale is > not dumped. See check_icu_locale.sh: > > In the old cluster: > SELECT collname, colliculocale FROM pg_collation WHERE collname = > 'testcoll_backwards' > collname | colliculocale > +--- > testcoll_backwards | @colBackwards=yes > (1 row) > > In the new cluster: > SELECT collname, colliculocale FROM pg_collation WHERE collname = > 'testcoll_backwards' > collname | colliculocale > +--- > testcoll_backwards | > (1 row) > > diff_dump_colliculocale.patch works for me. Ugh. Good catch, again! I have not tested the patch in details but this looks rather sane to me on a quick read. Peter? -- Michael signature.asc Description: PGP signature
Re: add checkpoint stats of snapshot and mapping files of pg_logical dir
On Wed, Aug 24, 2022 at 10:48:01AM +0900, Kyotaro Horiguchi wrote: > By the way, I think we use INSTR_TIME_* macros to do masure internal > durations (mainly for the monotonic clock characteristics, and to > reduce performance degradation on Windows?). I'm not sure that's > crutial here but I don't think there's any reason to use > GetCurrentTimestamp() instead. This implies two calls of gettimeofday(), but that does not worry me much in this code path. There is some consistency with CheckpointGuts() where we take timestamps for the sync requests. -- Michael signature.asc Description: PGP signature
Re: ecpg assertion on windows
Andres Freund writes: > On 2022-08-24 00:18:27 -0400, Tom Lane wrote: >> But if the regression tests are triggering use of uninitialized values, how >> could we have failed to detect that? Either valgrind or unstable behavior >> should have found this ages ago. > I think it's just different criteria for when to report issues. Valgrind > reports uninitialized memory only when there's a conditional branch depending > on it or such. Whereas this seems to trigger when passing an uninitialized > value to a function by value, even if it's then not relied upon. If the value is not actually relied on, then it's a false positive. I don't say we shouldn't fix it, because we routinely jump through hoops to silence various sorts of functionally-harmless warnings. But let's be clear about whether there's a real bug here. regards, tom lane
Re: SYSTEM_USER reserved word implementation
On Wed, Aug 17, 2022 at 04:48:42PM +0200, Drouvot, Bertrand wrote:
> That way one could test the SYSTEM_USER behavior without the need to have
> kerberos enabled.
I was looking at this patch and noticed that SYSTEM_USER returns a
"name", meaning that the value would be automatically truncated at 63
characters. We shouldn't imply that as authn_ids can be longer than
that, and this issue gets a bit worse once with the auth_method
appended to the string.
+if (!$use_unix_sockets)
+{
+ plan skip_all =>
+ "authentication tests cannot run without Unix-domain sockets";
+}
Are you sure that !$use_unix_sockets is safe here? Could we have
platforms where we use our port's getpeereid() with $use_unix_sockets
works? That would cause the test to fail with ENOSYS. Hmm. Without
being able to rely on HAVE_GETPEEREID, we could check for the error
generated when the fallback implementation does not work, and skip the
rest of the test.
--
Michael
signature.asc
Description: PGP signature
Re: ecpg assertion on windows
Hi, On 2022-08-24 00:18:27 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2022-08-23 20:36:55 -0700, Andres Freund wrote: > >> Running the ecpg regression tests interactively (to try to find a different > >> issue), triggered a crash on windows due to an uninitialized variable > >> (after > >> pressing "ignore" in that stupid gui window that we've only disabled for > >> the > >> backend). > >> "The variable 'replace_val' is being used without being initialized." > > > Looks to me like that's justified. > > Hmm ... that message sounded like it is a run-time detection not from > static analysis. Yes, it's a runtime error. > But if the regression tests are triggering use of uninitialized values, how > could we have failed to detect that? Either valgrind or unstable behavior > should have found this ages ago. I think it's just different criteria for when to report issues. Valgrind reports uninitialized memory only when there's a conditional branch depending on it or such. Whereas this seems to trigger when passing an uninitialized value to a function by value, even if it's then not relied upon. I don't think we regularly test all client tests with valgrind, btw. Skink only runs the server under valgrind at least. > Seeing that replace_val is a union of differently-sized types, > I was wondering if this message is a false positive based on > struct assignment transferring a few uninitialized bytes, or > something like that. I think it's genuinely uninitialized - if you track what happens if the first parameter is e.g. %X: It'll not initialize replace_val, but then call pgtypes_fmt_replace(). So an uninit value is passed. Greetings, Andres Freund
Re: ecpg assertion on windows
Andres Freund writes: > On 2022-08-23 20:36:55 -0700, Andres Freund wrote: >> Running the ecpg regression tests interactively (to try to find a different >> issue), triggered a crash on windows due to an uninitialized variable (after >> pressing "ignore" in that stupid gui window that we've only disabled for the >> backend). >> "The variable 'replace_val' is being used without being initialized." > Looks to me like that's justified. Hmm ... that message sounded like it is a run-time detection not from static analysis. But if the regression tests are triggering use of uninitialized values, how could we have failed to detect that? Either valgrind or unstable behavior should have found this ages ago. Seeing that replace_val is a union of differently-sized types, I was wondering if this message is a false positive based on struct assignment transferring a few uninitialized bytes, or something like that. regards, tom lane
Re: SQL/JSON features for v15
On Wed, Aug 24, 2022 at 11:55 AM Amit Langote wrote:
> On Wed, Aug 24, 2022 at 6:29 AM Nikita Glukhov
> wrote:
> > Here is my plan:
> >
> > 0. Take my last v7-0001 patch as a base. It already contains refactoring
> > of JsonCoercion code. (Fix 0002 is not needed anymore, because it is for
> > json[b] domains, which simply will not be supported.)
> >
> > 1. Replace JSON_COERCION_VIA_EXPR in JsonCoercion with new
> > JsonCoercionType(s) for hardcoded coercions.
> >
> > 2. Disable all non-JSON-compatible output types in coerceJsonFuncExpr().
> >
> > 3. Add missing safe type input functions for integers, numerics, and
> > maybe others.
> >
> > 4. Implement hardcoded coercions using these functions in
> > ExecEvalJsonExprCoercion().
> >
> > 5. Try to allow only constants (and also maybe column/parameter
> > references) in JSON_VALUE's DEFAULT expressions. This should be enough
> > for the most of practical cases. JSON_QUERY even does not have DEFAULT
> > expressions -- it has only EMPTY ARRAY and EMPTY OBJECT, which can be
> > treated as simple JSON constants.
> >
> > But it is possible to allow all other expressions in ERROR ON ERROR
> > case, and I don't know if it will be consistent enough to allow some
> > expressions in one case and deny in other.
> >
> > And there is another problem: expressions can be only checked for
> > Const-ness only after expression simplification. AFAIU, at the
> > parsing stage they look like 'string'::type. So, it's unclear if it
> > is correct to check expressions in ExecInitExpr().
>
> IIUC, the idea is to remove the support for `DEFAULT expression` in
> the following, no?
>
> json_value ( context_item, path_expression
> ...
> [ { ERROR | NULL | DEFAULT expression } ON EMPTY ]
> [ { ERROR | NULL | DEFAULT expression } ON ERROR ])
>
> json_query ( context_item, path_expression
> ...
> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> ON EMPTY ]
> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> ON ERROR ])
Or is the idea rather to restrict the set of data types we allow in `[
RETURNING data_type ]`?
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: use ARM intrinsics in pg_lfind32() where available
On Tue, Aug 23, 2022 at 4:15 AM Nathan Bossart wrote: > > On Mon, Aug 22, 2022 at 11:50:35AM +0700, John Naylor wrote: > > Is this also ever defined on 32-bit? If so, is it safe, meaning the > > compiler will not emit these instructions without additional flags? > > I'm wondering if __aarch64__ would be clearer on that, and if we get > > windows-on-arm support as has been proposed, could also add _M_ARM64. > > I haven't been able to enable __ARM_NEON on 32-bit, but if it is somehow > possible, we should probably add an __aarch64__ check since functions like > vmaxvq_u32() do not appear to be available on 32-bit. I have been able to > compile for __aarch64__ without __ARM_NEON, so it might still be a good > idea to check for __ARM_NEON. The important thing is: if we compile with __aarch64__ as a target: - Will the compiler emit the intended instructions from the intrinsics without extra flags? - Can a user on ARM64 ever get a runtime fault if the machine attempts to execute NEON instructions? "I have been able to compile for __aarch64__ without __ARM_NEON" doesn't really answer that question -- what exactly did this entail? > > I also see #if defined(__aarch64__) || defined(__aarch64) in our > > codebase already, but I'm not sure what recognizes the latter. > > I'm not sure what uses the latter, either. I took a quick look around at Debian code search, *BSD, Apple, and a few other places, and I can't find it. Then, I looked at the discussions around commit 5c7603c318872a42e "Add ARM64 (aarch64) support to s_lock.h", and the proposed patch [1] only had __aarch64__ . When it was committed, the platform was vaporware and I suppose we included "__aarch64" as a prophylactic measure because no other reason was given. It doesn't seem to exist anywhere, so unless someone can demonstrate otherwise, I'm going to rip it out soon. [1] https://www.postgresql.org/message-id/flat/1368448758.23422.12.camel%40t520.redhat.com -- John Naylor EDB: http://www.enterprisedb.com
Re: [PATCH] Expose port->authn_id to extensions and triggers
On Tue, Aug 23, 2022 at 10:04:30AM -0700, Jacob Champion wrote: > On 8/23/22 01:53, Drouvot, Bertrand wrote: >> @@ -2688,6 +2689,7 @@ InitProcessGlobals(void) >> >> MyProcPid = getpid(); >> >> MyStartTimestamp = GetCurrentTimestamp(); >> >> MyStartTime = timestamptz_to_time_t(MyStartTimestamp); >> >> + memset(, 0, sizeof(MyClientConnectionInfo)); >> >> >> >> /* >> >> * Set a different global seed in every process. We want something > > When can we rely on static initialization, and when can't we? Is there a > concern that the memory could have been polluted from before the > postmaster's fork? My main worry here is EXEC_BACKEND, where we would just use our own implementation of fork(), and it is a bad idea at the end to leave that untouched while we could have code paths that attempt to access it. At the end, I have moved the initialization at the same place as where we set MyProcPort for a backend in BackendInitialize(), mainly as a matter of consistency because ClientConnectionInfo is aimed at being a subset of that. And applied. -- Michael signature.asc Description: PGP signature
Re: Assertion failure on PG15 with modified test_shm_mq test
At Thu, 18 Aug 2022 16:58:24 +0530, Pavan Deolasee wrote in > Hi, > > On Thu, Aug 18, 2022 at 5:38 AM Andres Freund wrote: > > > We can't move pgstat shutdown into on_dsm callback because that's too late > > to > > allocate *new* dsm segments, which we might need to do while flushing > > out pending stats. > > > > See > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fa91d4c91f28f4819dc54f93adbd413a685e366a > > for a way to avoid the problem. > > > > > Thanks for the hint. I will try that approach. I wonder though if there is > something more we can do. For example, would it make sense to throw a > WARNING and avoid segfault if pgstat machinery is already shutdown? Just > worried if the code can be reached from multiple paths and testing all of > those would be difficult for extension developers, especially given this > may happen in error recovery path. I'm not sure how extensions can face this problem, but.. pgstat is designed not to lose reported numbers. The assertion is manifets that intention. It is not enabled on non-assertion builds and pgstat enters undefined state then maybe crash after the assertion point. On the other hand I don't think we want to perform the same check for the all places the assertion exists on non-assertion builds. We cannot simply replace the assertion with ereport(). regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: ecpg assertion on windows
Hi, On 2022-08-23 20:36:55 -0700, Andres Freund wrote: > Running the ecpg regression tests interactively (to try to find a different > issue), triggered a crash on windows due to an uninitialized variable (after > pressing "ignore" in that stupid gui window that we've only disabled for the > backend). > > "The variable 'replace_val' is being used without being initialized." Looks to me like that's justified. The paths in dttofmtasc_replace using PGTYPES_TYPE_NOTHING don't set replace_val, but call pgtypes_fmt_replace() - with replace_val passed by value. If that's the first replacement, an unitialized variable is passed... Seems either the caller should skip calling pgtypes_fmt_replace() in the NOTHING case, or replace_val should be zero initialized? - Andres
ecpg assertion on windows
Hi, I occasionally Running the ecpg regression tests interactively (to try to find a different issue), triggered a crash on windows due to an uninitialized variable (after pressing "ignore" in that stupid gui window that we've only disabled for the backend). "The variable 'replace_val' is being used without being initialized." Child-SP RetAddr Call Site 00b3`3bcfe140 7ff9`03f9cd74 libpgtypes!failwithmessage( void * retaddr = 0x7ff9`03f96133, int crttype = 0n1, int errnum = 0n3, char * msg = 0x00b3`3bcff050 "The variable 'replace_val' is being used without being initialized.")+0x234 [d:\a01\_work\12\s\src\vctools\crt\vcstartup\src\rtc\error.cpp @ 213] 00b3`3bcff030 7ff9`03f96133 libpgtypes!_RTC_UninitUse( char * varname = 0x7ff9`03fa8a90 "replace_val")+0xa4 [d:\a01\_work\12\s\src\vctools\crt\vcstartup\src\rtc\error.cpp @ 362] 00b3`3bcff470 7ff9`03f94acd libpgtypes!dttofmtasc_replace( int64 * ts = 0x00b3`3bcff778, long dDate = 0n0, int dow = 0n6, struct tm * tm = 0x00b3`3bcff598, char * output = 0x026e`9c223de0 "abc-00:00:00", int * pstr_len = 0x00b3`3bcff620, char * fmtstr = 0x7ff7`b01ae5c0 "abc-%X-def-%x-ghi%%")+0xe53 [C:\dev\postgres-meson\src\interfaces\ecpg\pgtypeslib\timestamp.c @ 759] *** WARNING: Unable to verify checksum for C:\dev\postgres-meson\build-msbuild\src\interfaces\ecpg\test\pgtypeslib\dt_test.exe 00b3`3bcff550 7ff7`b01a23c9 libpgtypes!PGTYPEStimestamp_fmt_asc( int64 * ts = 0x00b3`3bcff778, char * output = 0x026e`9c223de0 "abc-00:00:00", int str_len = 0n19, char * fmtstr = 0x7ff7`b01ae5c0 "abc-%X-def-%x-ghi%%")+0xed [C:\dev\postgres-meson\src\interfaces\ecpg\pgtypeslib\timestamp.c @ 794] 00b3`3bcff610 7ff7`b01a4499 dt_test!main(void)+0xe59 [C:\dev\postgres-meson\src\interfaces\ecpg\test\pgtypeslib\dt_test.pgc @ 200] 00b3`3bcff860 7ff7`b01a433e dt_test!invoke_main(void)+0x39 [d:\a01\_work\12\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 79] 00b3`3bcff8b0 7ff7`b01a41fe dt_test!__scrt_common_main_seh(void)+0x12e [d:\a01\_work\12\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 288] 00b3`3bcff920 7ff7`b01a452e dt_test!__scrt_common_main(void)+0xe [d:\a01\_work\12\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 331] 00b3`3bcff950 7ff9`1d987034 dt_test!mainCRTStartup( void * __formal = 0x00b3`3bbe8000)+0xe [d:\a01\_work\12\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp @ 17] 00b3`3bcff980 7ff9`1f842651 KERNEL32!BaseThreadInitThunk+0x14 00b3`3bcff9b0 ` ntdll!RtlUserThreadStart+0x21 I haven't analyzed this further. CI also shows ecpg itself occasionally crashing, but I haven't managed to catch it in the act. Greetings, Andres Freund
Re: SQL/JSON features for v15
Hi Nikita,
On Wed, Aug 24, 2022 at 6:29 AM Nikita Glukhov wrote:
> Here is my plan:
>
> 0. Take my last v7-0001 patch as a base. It already contains refactoring
> of JsonCoercion code. (Fix 0002 is not needed anymore, because it is for
> json[b] domains, which simply will not be supported.)
>
> 1. Replace JSON_COERCION_VIA_EXPR in JsonCoercion with new
> JsonCoercionType(s) for hardcoded coercions.
>
> 2. Disable all non-JSON-compatible output types in coerceJsonFuncExpr().
>
> 3. Add missing safe type input functions for integers, numerics, and
> maybe others.
>
> 4. Implement hardcoded coercions using these functions in
> ExecEvalJsonExprCoercion().
>
> 5. Try to allow only constants (and also maybe column/parameter
> references) in JSON_VALUE's DEFAULT expressions. This should be enough
> for the most of practical cases. JSON_QUERY even does not have DEFAULT
> expressions -- it has only EMPTY ARRAY and EMPTY OBJECT, which can be
> treated as simple JSON constants.
>
> But it is possible to allow all other expressions in ERROR ON ERROR
> case, and I don't know if it will be consistent enough to allow some
> expressions in one case and deny in other.
>
> And there is another problem: expressions can be only checked for
> Const-ness only after expression simplification. AFAIU, at the
> parsing stage they look like 'string'::type. So, it's unclear if it
> is correct to check expressions in ExecInitExpr().
IIUC, the idea is to remove the support for `DEFAULT expression` in
the following, no?
json_value ( context_item, path_expression
...
[ { ERROR | NULL | DEFAULT expression } ON EMPTY ]
[ { ERROR | NULL | DEFAULT expression } ON ERROR ])
json_query ( context_item, path_expression
...
[ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
ON EMPTY ]
[ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
ON ERROR ])
If that's the case, I'd imagine that `default_expr` in the following
will be NULL for now:
/*
* JsonBehavior -
* representation of JSON ON ... BEHAVIOR clause
*/
typedef struct JsonBehavior
{
NodeTag type;
JsonBehaviorType btype; /* behavior type */
Node *default_expr; /* default expression, if any */
} JsonBehavior;
And if so, no expression left to check the Const-ness of?
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: Strip -mmacosx-version-min options from plperl build
Andres Freund writes:
> FWIW, while trying to mirror the same logic in meson I learned that the new
> logic removes the rpath setting from the parameters on at least netbsd and
> suse. We'll add them back - unless --disable-rpath is used.
Hmm ... on my shiny new netbsd buildfarm animal, I see:
$ perl -MConfig -e 'print "$Config{ccdlflags}"'
-Wl,-E -Wl,-R/usr/pkg/lib/perl5/5.34.0/powerpc-netbsd-thread-multi/CORE
$ perl -MConfig -e 'print "$Config{ldflags}"'
-pthread -L/usr/lib -Wl,-R/usr/lib -Wl,-R/usr/pkg/lib -L/usr/pkg/lib
$ perl -MExtUtils::Embed -e ldopts
-Wl,-E -Wl,-R/usr/pkg/lib/perl5/5.34.0/powerpc-netbsd-thread-multi/CORE
-pthread -L/usr/lib -Wl,-R/usr/lib -Wl,-R/usr/pkg/lib -L/usr/pkg/lib
-L/usr/pkg/lib/perl5/5.34.0/powerpc-netbsd-thread-multi/CORE -lperl -lm -lcrypt
-lpthread
So we were *already* stripping the rpath for where libperl.so is,
and now we also strip -L and rpath for /usr/lib (which surely is
pointless) and for /usr/pkg/lib (which in point of fact have to
be added to the PG configuration options anyway). These Perl
options seem a bit inconsistent ...
> I think some
> distributions build with --disable-rpath. Not sure if worth worrying about.
I believe that policy only makes sense for distros that expect every
shared library to appear in /usr/lib (or /usr/lib64 perhaps). Red Hat
for one does that --- but they follow through: libperl.so is there.
I think we're good here, at least till somebody points out a platform
where we aren't.
regards, tom lane
Re: shadow variables - pg15 edition
On Wed, Aug 24, 2022 at 12:37:29PM +1200, David Rowley wrote:
> On Tue, 23 Aug 2022 at 14:14, Justin Pryzby wrote:
> > Actually, they didn't sneak in - what I sent are the patches which are
> > ready to
> > be reviewed, excluding the set of "this" and "tmp" and other renames which
> > you
> > disliked. In the branch (not the squished patch) the first ~15 patches were
> > mostly for C99 for loops - I presented them this way deliberately, so you
> > could
> > review and comment on whatever you're able to bite off, or run with whatever
> > parts you think are ready. I rewrote it now to be more bite sized by
> > truncating off the 2nd half of the patches.
>
> Thanks for the updated patch.
>
> I've now pushed it after making some small adjustments.
Thanks for handling them.
Attached are half of the remainder of what I've written, ready for review.
I also put it here:
https://github.com/justinpryzby/postgres/tree/avoid-shadow-vars
You may or may not find the associated commit messages to be useful.
Let me know if you'd like the individual patches included here, instead.
The first patch removes 2ndary, "inner" declarations, where that seems
reasonably safe and consistent with existing practice (and probably what the
original authors intended or would have written).
--
Justin
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 87b243e0d4b..a090cada400 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3017,46 +3017,45 @@ XLogFileInitInternal(XLogSegNo logsegno, TimeLineID
logtli,
}
pgstat_report_wait_end();
if (save_errno)
{
/*
* If we fail to make the file, delete it to release disk space
*/
unlink(tmppath);
close(fd);
errno = save_errno;
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not write to file \"%s\": %m",
tmppath)));
}
pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_SYNC);
if (pg_fsync(fd) != 0)
{
- int save_errno = errno;
-
+ save_errno = errno;
close(fd);
errno = save_errno;
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not fsync file \"%s\": %m",
tmppath)));
}
pgstat_report_wait_end();
if (close(fd) != 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not close file \"%s\": %m",
tmppath)));
/*
* Now move the segment into place with its final name. Cope with
* possibility that someone else has created the file while we were
* filling ours: if so, use ours to pre-create a future log segment.
*/
installed_segno = logsegno;
/*
* XXX: What should we use as max_segno? We used to use XLOGfileslop
when
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9be04c8a1e7..dacc989d855 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16777,45 +16777,44 @@ PreCommit_on_commit_actions(void)
oids_to_truncate =
lappend_oid(oids_to_truncate, oc->relid);
break;
case ONCOMMIT_DROP:
oids_to_drop = lappend_oid(oids_to_drop,
oc->relid);
break;
}
}
/*
* Truncate relations before dropping so that all dependencies between
* relations are removed after they are worked on. Doing it like this
* might be a waste as it is possible that a relation being truncated
will
* be dropped anyway due to its parent being dropped, but this makes the
* code more robust because of not having to re-check that the relation
* exists at truncation time.
*/
if (oids_to_truncate != NIL)
heap_truncate(oids_to_truncate);
if (oids_to_drop != NIL)
{
ObjectAddresses *targetObjects = new_object_addresses();
- ListCell *l;
foreach(l, oids_to_drop)
{
ObjectAddress object;
object.classId = RelationRelationId;
object.objectId = lfirst_oid(l);
object.objectSubId = 0;
Assert(!object_address_present(, targetObjects));
add_exact_object_address(, targetObjects);
}
/*
* Since this is an automatic drop, rather than one directly
Re: Strip -mmacosx-version-min options from plperl build
Hi, On 2022-08-22 08:37:40 -0700, Andres Freund wrote: > On 2022-08-22 16:32:36 +0200, Peter Eisentraut wrote: > > On 20.08.22 23:44, Andres Freund wrote: > > > On 2022-08-20 16:53:31 -0400, Tom Lane wrote: > > > > Andres Freund writes: > > > > > Maybe a daft question: Why do want any of the -l flags other than > > > > > -lperl? With > > > > > the patch configure spits out the following on my debian system: > > > > > > > > > checking for CFLAGS to compile embedded Perl... -DDEBIAN > > > > > checking for flags to link embedded Perl... > > > > > -L/usr/lib/x86_64-linux-gnu/perl/5.34/CORE -lperl -ldl -lm -lpthread > > > > > -lc -lcrypt > > > > > > > > > those libraries were likely relevant to build libperl, but don't look > > > > > relevant > > > > > for linking to it dynamically. > > > > > > > > I'm certain that there are/were platforms that insist on those libraries > > > > being mentioned anyway. Maybe they are all obsolete now? > > > > > > I don't think any of the supported platforms require it for stuff used > > > inside > > > the shared library (and we'd be in trouble if so, check e.g. libpq.pc). > > > But of > > > course that's different if there's inline function / macros getting > > > pulled in. > > > > > > Which turns out to be an issue on AIX. All the -l flags added by perl can > > > be > > > removed for xlc, but for gcc, -lpthreads (or -pthread) it is required. > > > > > > Tried it on Solaris (32 bit, not sure if there's a 64bit perl available), > > > works. > > > > Does that mean my proposed patch (v2) is adequate for these platforms, or > > does it need further analysis? > > I think it's a clear improvement over the status quo. Unnecessary -l's are > pretty harmless compared to random other flags. FWIW, while trying to mirror the same logic in meson I learned that the new logic removes the rpath setting from the parameters on at least netbsd and suse. We'll add them back - unless --disable-rpath is used. I think some distributions build with --disable-rpath. Not sure if worth worrying about. Greetings, Andres Freund
Re: Symbol referencing errors
Hi,
On 2022-08-23 01:34:36 -0700, Andres Freund wrote:
> On 2019-04-23 06:23:13 +0100, Andrew Gierth wrote:
> > I wonder if it's the use of -Bsymbolic that causes this (buildfarm logs
> > don't seem to go back far enough to check). (Note to original poster:
> > -Bsymbolic is there for a reason, you can't just remove it - but see
> > below.)
>
> For the record, yes, the "ld: warning: symbol referencing errors" warnings are
> due to -Bsymbolic while linking extensions. The man page says:
> "The link-editor issues warnings for undefined symbols unless -z defs
> overrides"
>
>
> > Since this is an ELF platform - arguably the closest thing to the
> > original reference ELF platform, at least by descent - it should not
> > require the kinds of tricks used on macOS and AIX; but we haven't done
> > the work needed to test using version scripts in place of -Bsymbolic for
> > fixing the symbol conflict problems. That ought to be a relatively
> > straightforward project for someone with access to a system to test on
> > (and I'm happy to advise on it).
>
> It's indeed trivial - the only change needed from linux is to replace
> -Wl,--version-script=... with -Wl,-M...
Patch attached. Passed check-world (without tap tests, didn't install the perl
mods) on solaris. Does anybody see a reason not to apply? Even just having
less noisy build logs seem like an advantage.
Greetings,
Andres Freund
>From 176c8c31a74e01d3005cc7df6bae5fcd951b2f0c Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Tue, 23 Aug 2022 01:50:29 -0700
Subject: [PATCH v1] solaris: Use versioning scripts instead of -Bsymbolic
-Bsymbolic causes a lot of "ld: warning: symbol referencing errors"
warnings. It's quite easy to add the symbol versioning script, we just need a
slightly different parameter name.
Discussion: https://postgr.es/m/20220823083436.whtntk3bn3qpn...@awork3.anarazel.de
---
src/Makefile.shlib | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/Makefile.shlib b/src/Makefile.shlib
index 2af6192f0f3..3202b2e67de 100644
--- a/src/Makefile.shlib
+++ b/src/Makefile.shlib
@@ -188,10 +188,15 @@ ifeq ($(PORTNAME), linux)
endif
ifeq ($(PORTNAME), solaris)
- LINK.shared = $(COMPILER) -shared -Wl,-Bsymbolic
+ LINK.shared = $(COMPILER) -shared
ifdef soname
LINK.shared += -Wl,-soname,$(soname)
endif
+ BUILD.exports = ( echo '{ global:'; $(AWK) '/^[^\#]/ {printf "%s;\n",$$1}' $<; echo ' local: *; };' ) >$@
+ exports_file = $(SHLIB_EXPORTS:%.txt=%.list)
+ ifneq (,$(exports_file))
+LINK.shared += -Wl,-M$(exports_file)
+ endif
endif
ifeq ($(PORTNAME), cygwin)
--
2.37.0.3.g30cc8d0f14
Re: Inconsistencies around defining FRONTEND
Hi, On 2022-08-23 19:50:00 -0400, Tom Lane wrote: > Robert Haas writes: > > On Tue, Aug 23, 2022 at 7:24 PM Andres Freund wrote: > >> Just to make sure I understand - you're just trying to get rid of the > >> #define > >> frontends, not the -DFRONTENDs passed in from the Makefile? Because afaics > >> we > >> still need those, correct? > > > Oh, yeah, this only fixes the #define ones. But maybe fixing the other > > ones with a similar approach would be possible? > > > I really don't see why we should tolerate having #define FRONTEND in > > more than once place. > > src/port and src/common really need to do it like that (ie pass in > the -D switch) so that the identical source file can be built > both ways. Maybe we could get rid of -DFRONTEND in other places, > like pg_rewind and pg_waldump. We could, if we make xlogreader.c and the rmgrdesc routines built as part of src/common. I don't really see how otherwise. Greetings, Andres Freund
Re: add checkpoint stats of snapshot and mapping files of pg_logical dir
At Tue, 23 Aug 2022 10:49:40 -0700, Nathan Bossart wrote in > On Wed, Aug 17, 2022 at 11:17:24AM +0530, Bharath Rupireddy wrote: > > + "logical decoding file(s) > > processing time=%ld.%03d s", > > I would suggest shortening this to something like "logical decoding > processing" or "logical replication processing." > > > CheckPointRelationMap(); > > CheckPointReplicationSlots(); > > + > > + CheckpointStats.l_dec_ops_start_t = GetCurrentTimestamp(); > > CheckPointSnapBuild(); > > CheckPointLogicalRewriteHeap(); > > + CheckpointStats.l_dec_ops_end_t = GetCurrentTimestamp(); > > + > > CheckPointReplicationOrigin(); > > Shouldn't we include CheckPointReplicationSlots() and > CheckPointReplicationOrigin() in this new stat? By the way, I think we use INSTR_TIME_* macros to do masure internal durations (mainly for the monotonic clock characteristics, and to reduce performance degradation on Windows?). I'm not sure that's crutial here but I don't think there's any reason to use GetCurrentTimestamp() instead. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Add find_in_log() and advance_wal() perl functions to core test framework (?)
At Tue, 16 Aug 2022 18:40:49 +0200, Alvaro Herrera wrote in > On 2022-Aug-16, Andrew Dunstan wrote: > > > I don't think there's a hard and fast rule about it. Certainly the case > > would be more compelling if the functions were used across different TAP > > suites. The SSL suite has suite-specific modules. That's a pattern also > > worth considering. e.g something like. > > > > use FindBin qw($Bin); > > use lib $Bin; > > use MySuite; > > > > and then you put your common routines in MySuite.pm in the same > > directory as the TAP test files. > > Yeah, I agree with that for advance_wal. Regarding find_in_log, that > one seems general enough to warrant being in Cluster.pm -- consider > issues_sql_like, which also slurps_file($log). That could be unified a > little bit, I think. +1 regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: logical decoding and replication of sequences
On Wed, Aug 24, 2022 at 3:04 AM Robert Haas wrote: > I haven't been paying attention to this thread, but my attention was > just drawn to it, and I'm wondering if the issue you're trying to > track down here is actually the same as what I reported yesterday > here: > > https://www.postgresql.org/message-id/CA+TgmoY0Lri=fCueg7m_2R_bSspUb1F8OFycEGaHNJw_EUW-=q...@mail.gmail.com Summarising a chat we had about this: Different bug, similar ingredients. Robert describes a screw-up in what is written, but here we're talking about a cache invalidation bug while reading.
Re: shadow variables - pg15 edition
On Tue, 23 Aug 2022 at 14:14, Justin Pryzby wrote:
> Actually, they didn't sneak in - what I sent are the patches which are ready
> to
> be reviewed, excluding the set of "this" and "tmp" and other renames which you
> disliked. In the branch (not the squished patch) the first ~15 patches were
> mostly for C99 for loops - I presented them this way deliberately, so you
> could
> review and comment on whatever you're able to bite off, or run with whatever
> parts you think are ready. I rewrote it now to be more bite sized by
> truncating off the 2nd half of the patches.
Thanks for the updated patch.
I've now pushed it after making some small adjustments.
It seems there was one leftover rename still there, I removed that.
The only other changes I made were to just make the patch mode
consistent with what it was doing. There were a few cases where you
were doing:
if (typlen == -1) /* varlena */
{
- int i;
-
- for (i = 0; i < nvalues; i++)
+ for (int i = 0; i < nvalues; i++)
That wasn't really required to remove the warning as you'd already
adjusted the scope of the shadowed variable so there was no longer a
collision. The reason I adjusted these was because sometimes you were
doing that, and sometimes you were not. I wanted to be consistent, so
I opted for not doing it as it's not required for this effort. Maybe
one day those can be changed in some other unrelated effort to C99ify
our code.
The attached patch is just the portions I didn't commit.
Thanks for working on this.
David
diff --git a/src/backend/access/brin/brin_minmax_multi.c
b/src/backend/access/brin/brin_minmax_multi.c
index 524c1846b8..a581659fe2 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -620,14 +620,18 @@ brin_range_serialize(Ranges *range)
*/
if (typlen == -1) /* varlena */
{
- for (int i = 0; i < nvalues; i++)
+ int i;
+
+ for (i = 0; i < nvalues; i++)
{
len += VARSIZE_ANY(range->values[i]);
}
}
else if (typlen == -2) /* cstring */
{
- for (int i = 0; i < nvalues; i++)
+ int i;
+
+ for (i = 0; i < nvalues; i++)
{
/* don't forget to include the null terminator ;-) */
len += strlen(DatumGetCString(range->values[i])) + 1;
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index f9d40fa1a0..1545ff9f16 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1650,16 +1650,16 @@ interpret_ident_response(const char *ident_response,
return false;
else
{
- int j;
/* Index into *ident_user */
+ int i;
/* Index into *ident_user */
cursor++; /* Go over
colon */
while (pg_isblank(*cursor))
cursor++; /* skip
blanks */
/* Rest of line is user name.
Copy it over. */
- j = 0;
+ i = 0;
while (*cursor != '\r' && i <
IDENT_USERNAME_MAX)
- ident_user[j++] =
*cursor++;
- ident_user[j] = '\0';
+ ident_user[i++] =
*cursor++;
+ ident_user[i] = '\0';
return true;
}
}
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 91b9635dc0..6eeacb0d47 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1861,6 +1861,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
{
/* AND/OR clause, with all subclauses being compatible
*/
+ int i;
BoolExpr *bool_clause = ((BoolExpr *) clause);
List *bool_clauses = bool_clause->args;
@@ -1879,7 +1880,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
* current one. We need to consider if we're evaluating
AND or OR
* condition when merging the results.
*/
- for (int i
Re: Inconsistencies around defining FRONTEND
Robert Haas writes: > On Tue, Aug 23, 2022 at 7:24 PM Andres Freund wrote: >> Just to make sure I understand - you're just trying to get rid of the #define >> frontends, not the -DFRONTENDs passed in from the Makefile? Because afaics we >> still need those, correct? > Oh, yeah, this only fixes the #define ones. But maybe fixing the other > ones with a similar approach would be possible? > I really don't see why we should tolerate having #define FRONTEND in > more than once place. src/port and src/common really need to do it like that (ie pass in the -D switch) so that the identical source file can be built both ways. Maybe we could get rid of -DFRONTEND in other places, like pg_rewind and pg_waldump. regards, tom lane
Re: predefined role(s) for VACUUM and ANALYZE
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Tue, Jul 26, 2022 at 1:50 PM David G. Johnston > wrote: > >> Still, it seems somewhat appealing to give > >> people fine-grained control over this, rather than just "on" or "off". > > Appealing enough to consume a couple of permission bits? > > https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com > > I think we're down to 0 remaining now, so it'd be hard to justify > consuming 2 of 0 remaining bits. However, I maintain that the solution > to this is either (1) change the aclitem representation to get another > 32 bits or (2) invent a different system for less-commonly used > permission bits. Checking permissions for SELECT or UPDATE has to be > really fast, because most queries will need to do that sort of thing. > If we represented VACUUM or ANALYZE in some other way in the catalogs > that was more scalable but less efficient, it wouldn't be a big deal > (although there's the issue of code duplication to consider). I've long felt that we should redefine the way the ACLs work to have a distinct set of bits for each object type. We don't need to support a CONNECT bit on a table, yet we do today and we expend quite a few bits in that way. Having that handled on a per-object-type basis instead would allow us to get quite a bit more mileage out of the existing 32bit field before having to introduce more complicated storage methods like using a bit to tell us to go look up more ACLs somewhere else. Thanks, Stephen signature.asc Description: PGP signature
Re: Inconsistencies around defining FRONTEND
On Tue, Aug 23, 2022 at 7:24 PM Andres Freund wrote: > Just to make sure I understand - you're just trying to get rid of the #define > frontends, not the -DFRONTENDs passed in from the Makefile? Because afaics we > still need those, correct? Oh, yeah, this only fixes the #define ones. But maybe fixing the other ones with a similar approach would be possible? I really don't see why we should tolerate having #define FRONTEND in more than once place. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Inconsistencies around defining FRONTEND
Hi, On 2022-08-23 17:24:30 -0400, Robert Haas wrote: > On Sat, Aug 20, 2022 at 3:46 PM Andres Freund wrote: > > Unfortunately, the remaining uses of FRONTEND are required. That's: > > - pg_controldata, via #define > > - pg_resetwal, via #define > > - pg_rewind, via -DFRONTEND, due to xlogreader.c > > - pg_waldump, via #define and -DFRONTEND, due to xlogreader.c, xlogstats.c, > > rmgrdesc/*desc.c > > Actually, I think we could fix these pretty easily too. I just meant that they're not already obsolete ;) > See attached. Just to make sure I understand - you're just trying to get rid of the #define frontends, not the -DFRONTENDs passed in from the Makefile? Because afaics we still need those, correct? Greetings, Andres Freund
Re: Inconsistencies around defining FRONTEND
On Tue, Aug 23, 2022 at 5:56 PM Tom Lane wrote: > Robert Haas writes: > > Actually, I think we could fix these pretty easily too. See attached. > > Hmm, do these headers still pass headerscheck/cpluspluscheck? I didn't check before sending the patch, but now I ran it locally, and I did get failures from both, but they all seem to be unrelated. Mainly, it's sad that I don't have Python.h, but I didn't configure with python, so whatever. > I might quibble a bit with the exact placement of the #ifndef FRONTEND > tests, but overall this looks pretty plausible. Yep, that's arguable. In particular, should the redo functions also be protected by #ifdef FRONTEND? I'd be more than thrilled if you wanted to adjust this to taste and apply it, barring objections from others of course. -- Robert Haas EDB: http://www.enterprisedb.com
Re: sockaddr_un.sun_len vs. reality
On Tue, Aug 23, 2022 at 3:43 PM Tom Lane wrote: > Thomas Munro writes: > > I was nerd-sniped by the historical context of this single line of > > code. I'd already wondered many times (not just in PostgreSQL) > > whether and when that became a cargo-cult practice, replicated from > > other software and older books like Stevens. I failed to find any > > sign of an OS that needs it today, or likely even needed it this > > millennium. Now I'd like to propose removing it. > > Seems worth a try. Pushed, and build farm looks good. For the benefit of anyone else researching this topic, I should add that Stevens in fact said it's OK to skip this, and if I had opened UNIX Network Programming (3rd ed) volume I to page 99 I could have saved myself some time: "Even if the length field is present, we need never set it and need never examine it, unless we are dealing with routing sockets ...".
Re: SQL/JSON features for v15
On 2022-08-23 Tu 17:29, Nikita Glukhov wrote: > > > On 23.08.2022 22:31, Jonathan S. Katz wrote: >> On 8/23/22 2:10 PM, Andrew Dunstan wrote: >>> >>> On 2022-08-23 Tu 13:24, Tom Lane wrote: "Jonathan S. Katz" writes: > I saw Andrew suggest that the controversial parts of the patchset > may be > severable from some of the new functionality, so I would like to see > that proposal and if it is enough to overcome concerns. It's an interesting suggestion. Do people have the cycles available to make it happen in the next few days? >>> I will make time although probably Nikita and/or Amit would be quicker >>> than I would be. >> >> If you all can, you have my +1 to try it and see what folks think. > I am ready to start hacking now, but it's already night in Moscow, so > any result will be only tomorrow. > > Here is my plan: > > 0. Take my last v7-0001 patch as a base. It already contains refactoring > of JsonCoercion code. (Fix 0002 is not needed anymore, because it is for > json[b] domains, which simply will not be supported.) > > 1. Replace JSON_COERCION_VIA_EXPR in JsonCoercion with new > JsonCoercionType(s) for hardcoded coercions. > > 2. Disable all non-JSON-compatible output types in coerceJsonFuncExpr(). > > 3. Add missing safe type input functions for integers, numerics, and > maybe others. > > 4. Implement hardcoded coercions using these functions in > ExecEvalJsonExprCoercion(). > > 5. Try to allow only constants (and also maybe column/parameter > references) in JSON_VALUE's DEFAULT expressions. This should be enough > for the most of practical cases. JSON_QUERY even does not have DEFAULT > expressions -- it has only EMPTY ARRAY and EMPTY OBJECT, which can be > treated as simple JSON constants. er, really? This is from the regression output: SELECT JSON_QUERY(jsonb '[]', '$[*]' DEFAULT '"empty"' ON EMPTY); json_query "empty" (1 row) SELECT JSON_QUERY(jsonb '[1,2]', '$[*]' DEFAULT '"empty"' ON ERROR); json_query "empty" (1 row) > > But it is possible to allow all other expressions in ERROR ON ERROR > case, and I don't know if it will be consistent enough to allow some > expressions in one case and deny in other. > > And there is another problem: expressions can be only checked for > Const-ness only after expression simplification. AFAIU, at the > parsing stage they look like 'string'::type. So, it's unclear if it > is correct to check expressions in ExecInitExpr(). > > 6. Remove subtransactions. > Sounds like a good plan, modulo the issues in item 5. I would rather lose some features temporarily than try to turn handsprings to make them work and jeopardize the rest. I'll look forward to seeing your patch in the morning :-) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Inconsistencies around defining FRONTEND
Robert Haas writes: > Actually, I think we could fix these pretty easily too. See attached. Hmm, do these headers still pass headerscheck/cpluspluscheck? I might quibble a bit with the exact placement of the #ifndef FRONTEND tests, but overall this looks pretty plausible. regards, tom lane
Re: SQL/JSON features for v15
On 23.08.2022 22:31, Jonathan S. Katz wrote: On 8/23/22 2:10 PM, Andrew Dunstan wrote: On 2022-08-23 Tu 13:24, Tom Lane wrote: "Jonathan S. Katz" writes: I saw Andrew suggest that the controversial parts of the patchset may be severable from some of the new functionality, so I would like to see that proposal and if it is enough to overcome concerns. It's an interesting suggestion. Do people have the cycles available to make it happen in the next few days? I will make time although probably Nikita and/or Amit would be quicker than I would be. If you all can, you have my +1 to try it and see what folks think. I am ready to start hacking now, but it's already night in Moscow, so any result will be only tomorrow. Here is my plan: 0. Take my last v7-0001 patch as a base. It already contains refactoring of JsonCoercion code. (Fix 0002 is not needed anymore, because it is for json[b] domains, which simply will not be supported.) 1. Replace JSON_COERCION_VIA_EXPR in JsonCoercion with new JsonCoercionType(s) for hardcoded coercions. 2. Disable all non-JSON-compatible output types in coerceJsonFuncExpr(). 3. Add missing safe type input functions for integers, numerics, and maybe others. 4. Implement hardcoded coercions using these functions in ExecEvalJsonExprCoercion(). 5. Try to allow only constants (and also maybe column/parameter references) in JSON_VALUE's DEFAULT expressions. This should be enough for the most of practical cases. JSON_QUERY even does not have DEFAULT expressions -- it has only EMPTY ARRAY and EMPTY OBJECT, which can be treated as simple JSON constants. But it is possible to allow all other expressions in ERROR ON ERROR case, and I don't know if it will be consistent enough to allow some expressions in one case and deny in other. And there is another problem: expressions can be only checked for Const-ness only after expression simplification. AFAIU, at the parsing stage they look like 'string'::type. So, it's unclear if it is correct to check expressions in ExecInitExpr(). 6. Remove subtransactions. -- Nikita Glukhov Postgres Professional:http://www.postgrespro.com The Russian Postgres Company
Re: Inconsistencies around defining FRONTEND
On Sat, Aug 20, 2022 at 3:46 PM Andres Freund wrote: > Unfortunately, the remaining uses of FRONTEND are required. That's: > - pg_controldata, via #define > - pg_resetwal, via #define > - pg_rewind, via -DFRONTEND, due to xlogreader.c > - pg_waldump, via #define and -DFRONTEND, due to xlogreader.c, xlogstats.c, > rmgrdesc/*desc.c Actually, I think we could fix these pretty easily too. See attached. This has been annoying me for a while. I hope we can agree on a way to clean it up. -- Robert Haas EDB: http://www.enterprisedb.com v1-0001-Remove-define-FRONTEND-hacks.patch Description: Binary data
Re: Move NON_EXEC_STATIC from c.h
Peter Eisentraut writes: > Here is a new patch with more comments. LGTM regards, tom lane
Re: SQL/JSON features for v15
Andrew Dunstan writes: > On 2022-08-23 Tu 15:32, Jonathan S. Katz wrote: >> On 8/23/22 1:26 PM, Andres Freund wrote: >>> We could decide to revert this for 15, but leave it in tree for HEAD. >> If it comes to that, I think that is a reasonable suggestion so long >> as we're committed to making the requisite changes. I'm not particularly on board with that. In the first place, I'm unconvinced that very much of the current code will survive, and I don't want people contorting the rewrite in order to salvage committed code that would be better off junked. In the second place, if we still don't have a shippable feature in a year, then undoing it again is going to be just that much harder. > One good reason for this is that way we're not fighting against the node > changes, which complicate any reversion significantly. Having said that, I'm prepared to believe that a lot of the node infrastructure won't change because it's dictated by the SQL-spec grammar. So we could leave that part alone in HEAD; at worst it adds some dead code in backend/nodes. regards, tom lane
Re: SQL/JSON features for v15
út 23. 8. 2022 v 21:54 odesílatel Tom Lane napsal: > Andres Freund writes: > > On 2022-08-23 13:28:50 -0400, Tom Lane wrote: > >> I agree with the upthread comments that we only need/want to catch > >> foreseeable incorrect-input errors, and that the way to make that > >> happen is to refactor the related type input functions, and that > >> a lot of the heavy lifting for that has been done already. > > > I think it's a good direction to go in. What of the heavy lifting for > that has > > been done already? I'd have guessed that the hard part is to add > different, > > optional, type input, type coercion signatures, and then converting a > lot of > > types to that? > > I was assuming that we would only bother to do this for a few core types. > Of those, at least the datetime types were already done for previous > JSON-related features. If we want extensibility, then as Robert said > there's going to have to be work done to create a common API that type > input functions can implement, which seems like a pretty heavy lift. > We could get it done for v16 if we start now, I imagine. > +1 Pavel > regards, tom lane > > >
Re: SQL/JSON features for v15
Andres Freund writes: > On 2022-08-23 13:28:50 -0400, Tom Lane wrote: >> I agree with the upthread comments that we only need/want to catch >> foreseeable incorrect-input errors, and that the way to make that >> happen is to refactor the related type input functions, and that >> a lot of the heavy lifting for that has been done already. > I think it's a good direction to go in. What of the heavy lifting for that has > been done already? I'd have guessed that the hard part is to add different, > optional, type input, type coercion signatures, and then converting a lot of > types to that? I was assuming that we would only bother to do this for a few core types. Of those, at least the datetime types were already done for previous JSON-related features. If we want extensibility, then as Robert said there's going to have to be work done to create a common API that type input functions can implement, which seems like a pretty heavy lift. We could get it done for v16 if we start now, I imagine. regards, tom lane
Re: SQL/JSON features for v15
On 2022-08-23 Tu 15:32, Jonathan S. Katz wrote: > On 8/23/22 1:26 PM, Andres Freund wrote: >> Hi, >> >> On 2022-08-23 13:18:49 -0400, Jonathan S. Katz wrote: >>> Taking RMT hat off, if the outcome is "revert", I do want to ensure >>> we don't >>> lose momentum on getting this into v16. I know a lot of time and >>> effort has >>> gone into this featureset and it seems to be trending in the right >>> direction. We have a mixed history on reverts in terms of if/when >>> they are >>> committed and I don't want to see that happen to these features. I >>> do think >>> this will remain a headline feature even if we delay it for v16. >> >> We could decide to revert this for 15, but leave it in tree for HEAD. > > If it comes to that, I think that is a reasonable suggestion so long > as we're committed to making the requisite changes. > > One good reason for this is that way we're not fighting against the node changes, which complicate any reversion significantly. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: SQL/JSON features for v15
On 8/23/22 1:26 PM, Andres Freund wrote: Hi, On 2022-08-23 13:18:49 -0400, Jonathan S. Katz wrote: Taking RMT hat off, if the outcome is "revert", I do want to ensure we don't lose momentum on getting this into v16. I know a lot of time and effort has gone into this featureset and it seems to be trending in the right direction. We have a mixed history on reverts in terms of if/when they are committed and I don't want to see that happen to these features. I do think this will remain a headline feature even if we delay it for v16. We could decide to revert this for 15, but leave it in tree for HEAD. If it comes to that, I think that is a reasonable suggestion so long as we're committed to making the requisite changes. Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: SQL/JSON features for v15
On 8/23/22 2:10 PM, Andrew Dunstan wrote: On 2022-08-23 Tu 13:24, Tom Lane wrote: "Jonathan S. Katz" writes: I saw Andrew suggest that the controversial parts of the patchset may be severable from some of the new functionality, so I would like to see that proposal and if it is enough to overcome concerns. It's an interesting suggestion. Do people have the cycles available to make it happen in the next few days? I will make time although probably Nikita and/or Amit would be quicker than I would be. If you all can, you have my +1 to try it and see what folks think. Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: Move NON_EXEC_STATIC from c.h
On 16.08.22 15:50, Tom Lane wrote: Peter Eisentraut writes: Looking to tidy up c.h a bit, I think the NON_EXEC_STATIC #define doesn't need to be known globally, and it's not related to establishing a portable C environment, so I propose to move it to a more localized header, such as postmaster.h, as in the attached patch. Hmm, postgres.h seems like a better choice, since in principle any backend file might need this. This arrangement could require postmaster.h to be included just for this macro. I picked postmaster.h because the other side of the code, where the no-longer-static symbols are used, is in postmaster.c. But postgres.h is also ok. Also, the macro was severely underdocumented already, and I don't find "no comment at all" to be better. Can't we afford a couple of lines of explanation? Here is a new patch with more comments.From 06f30dcf46ed4f89e0c9c5e67615a5c9ad492988 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Tue, 23 Aug 2022 20:24:21 +0200 Subject: [PATCH v2] Move NON_EXEC_STATIC from c.h to postgres.h It is not needed at the scope of c.h, only in backend code. Discussion: https://www.postgresql.org/message-id/flat/a6a6b48e-ca0a-b58d-18de-98e40d94b842%40enterprisedb.com --- src/include/c.h| 7 --- src/include/postgres.h | 20 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/include/c.h b/src/include/c.h index dfc366b026..aad9554de6 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -1338,13 +1338,6 @@ typedef intptr_t sigjmp_buf[5]; #endif /* __MINGW64__ */ #endif /* WIN32 */ -/* EXEC_BACKEND defines */ -#ifdef EXEC_BACKEND -#define NON_EXEC_STATIC -#else -#define NON_EXEC_STATIC static -#endif - /* /port compatibility functions */ #include "port.h" diff --git a/src/include/postgres.h b/src/include/postgres.h index 31358110dc..13903fa022 100644 --- a/src/include/postgres.h +++ b/src/include/postgres.h @@ -25,6 +25,7 @@ * --- * 1) variable-length datatypes (TOAST support) * 2) Datum type + support macros + * 3) miscellaneous * * NOTES * @@ -805,4 +806,23 @@ extern Datum Float8GetDatum(float8 X); #define Float8GetDatumFast(X) PointerGetDatum(&(X)) #endif + +/* + * Section 3: miscellaneous + * + */ + +/* + * NON_EXEC_STATIC: It's sometimes useful to define a variable or function + * that is normally static but extern when using EXEC_BACKEND (see + * pg_config_manual.h). There would then typically be some code in + * postmaster.c that uses those extern symbols to transfer state between + * processes or do whatever other things it needs to do in EXEC_BACKEND mode. + */ +#ifdef EXEC_BACKEND +#define NON_EXEC_STATIC +#else +#define NON_EXEC_STATIC static +#endif + #endif /* POSTGRES_H */ -- 2.37.1
Re: replacing role-level NOINHERIT with a grant-level option
On Thu, Jul 28, 2022 at 12:32 PM tushar wrote: > On 7/28/22 8:03 PM, Robert Haas wrote: > > No, it seems to me that's behaving as intended. REVOKE BLAH OPTION ... > > is intended to be a way of switching an option off. > Ok, Thanks, Robert. I tested with a couple of more scenarios like > pg_upgrade/pg_dumpall /grant/revoke .. with admin option/inherit > and things look good to me. This patch needed to be rebased pretty extensively after commit ce6b672e4455820a0348214be0da1a024c3f619f. Here is a new version. -- Robert Haas EDB: http://www.enterprisedb.com v6-0001-Allow-grant-level-control-of-role-inheritance-beh.patch Description: Binary data
Re: SQL/JSON features for v15
Hi, On 2022-08-23 13:28:50 -0400, Tom Lane wrote: > I agree with the upthread comments that we only need/want to catch > foreseeable incorrect-input errors, and that the way to make that > happen is to refactor the related type input functions, and that > a lot of the heavy lifting for that has been done already. I think it's a good direction to go in. What of the heavy lifting for that has been done already? I'd have guessed that the hard part is to add different, optional, type input, type coercion signatures, and then converting a lot of types to that? Greetings, Andres Freund
Re: SQL/JSON features for v15
Hi, On 2022-08-23 13:33:42 -0400, Robert Haas wrote: > On Tue, Aug 23, 2022 at 1:23 PM Andres Freund wrote: > > > But that's exactly what I'm complaining about. Catching an error that > > > unwound a bunch of stack frames where complicated things are happening > > > is fraught with peril. There's probably a bunch of errors that could > > > be thrown from somewhere in that code - out of memory being a great > > > example - that should not be caught. > > > > The code as is handles this to some degree. Only ERRCODE_DATA_EXCEPTION, > > ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION are caught, the rest is immediately > > rethrown. > > AFAIK, Tom has rejected every previous effort to introduce this type > of coding into the tree rather forcefully. What makes it OK now? I didn't say it was! I don't like it much - I was just saying that it handles that case to some degree. > > I'm not sure what the general alternative is though. Part of the feature is > > generating a composite type from json - there's just no way we can make all > > possible coercion pathways not error out. That'd necessitate requiring all > > builtin types and extensions types out there to provide input functions that > > don't throw on invalid input and all coercions to not throw either. That > > just > > seems unrealistic. > > Well, I think that having input functions report input that is not > valid for the data type in some way other than just chucking an error > as they'd also do for a missing TOAST chunk would be a pretty sensible > plan. I'd support doing that if we forced a hard compatibility break, > and I'd support that if we provided some way for old code to continue > running in degraded mode. I haven't thought too much about the > coercion case, but I suppose the issues are similar. What I don't > support is saying -- well, upgrading our infrastructure is hard, so > let's just kludge it. I guess the 'degraded mode' approach is kind of what I was trying to describe with: > I think the best we could without subtransactions do perhaps is to add > metadata to pg_cast, pg_type telling us whether certain types of errors are > possible, and requiring ERROR ON ERROR when coercion paths are required that > don't have those options. Greetings, Andres Freund
Re: SQL/JSON features for v15
On 23.08.2022 20:38, Pavel Stehule wrote:
út 23. 8. 2022 v 19:27 odesílatel Andres Freund
napsal:
Hi,
On 2022-08-23 18:06:22 +0200, Pavel Stehule wrote:
> The errors that should be handled are related to json structure
errors. I
> don't think so we have to handle all errors and all conversions.
>
> The JSON knows only three types - and these conversions can be
written
> specially for this case - or we can write json io routines to be
able to
> signal error
> without an exception.
I think that's not true unfortunately. You can specify return
types, and
composite types can be populated. Which essentially requires arbitrary
coercions.
Please, can you send an example? Maybe we try to fix a feature that is
not required by standard.
- Returning arbitrary types in JSON_VALUE using I/O coercion
from JSON string (more precisely, text::arbitrary_type cast):
SELECT JSON_QUERY(jsonb '"1, 2"', '$' RETURNING point);
json_query
(1,2)
(1 row)
- Returning composite and array types in JSON_QUERY, which is implemented
reusing the code of our json[b]_populate_record[set]():
SELECT JSON_QUERY(jsonb '[1, "2", null]', '$' RETURNING int[]);
json_query
{1,2,NULL}
(1 row)
--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company
Re: SQL/JSON features for v15
On 2022-08-23 Tu 13:24, Tom Lane wrote: > "Jonathan S. Katz" writes: >> I saw Andrew suggest that the controversial parts of the patchset may be >> severable from some of the new functionality, so I would like to see >> that proposal and if it is enough to overcome concerns. > It's an interesting suggestion. Do people have the cycles available > to make it happen in the next few days? > > I will make time although probably Nikita and/or Amit would be quicker than I would be. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: handling multiple matching constraints in DetachPartitionFinalize()
On 2022-Aug-23, Zhihong Yu wrote: > Toggling enable_seqscan on / off using the example from `parenting a PK > constraint to a self-FK one` thread, it can be shown that different > constraint Id would be detached which is incorrect. > However, I am not sure whether toggling enable_seqscan mid-test is > legitimate. Well, let's see it in action. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
On Wed, Aug 17, 2022 at 12:39:26PM +0530, Bharath Rupireddy wrote: > + /* > + * We're only handling directories here, skip if it's not ours. Also, > skip > + * if the caller has already performed this check. > + */ > + if (!slot_dir_checked && > + lstat(path, ) == 0 && !S_ISDIR(statbuf.st_mode)) > return; There was previous discussion about removing this stanza from ReorderBufferCleanupSeralizedTXNs() completely [0]. If that is still possible, I think I would choose that over having callers specify whether to do the directory check. > + /* we're only handling directories here, skip if it's not one */ > + sprintf(path, "pg_replslot/%s", logical_de->d_name); > + if (get_dirent_type(path, logical_de, false, LOG) != > PGFILETYPE_DIR) > + continue; IIUC an error in get_dirent_type() could cause slots to be skipped here, which is a behavior change. [0] https://postgr.es/m/20220329224832.GA560657%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: handling multiple matching constraints in DetachPartitionFinalize()
On Tue, Aug 23, 2022 at 10:53 AM Alvaro Herrera wrote: > On 2022-Aug-23, Zhihong Yu wrote: > > > This is what I came up with. > > I suggest you provide a set of SQL commands that provoke some wrong > behavior with the original code, and show that they generate good > behavior after the patch. Otherwise, it's hard to evaluate the > usefulness of this. > > -- > Álvaro HerreraBreisgau, Deutschland — > https://www.EnterpriseDB.com/ > "Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente" > Toggling enable_seqscan on / off using the example from `parenting a PK constraint to a self-FK one` thread, it can be shown that different constraint Id would be detached which is incorrect. However, I am not sure whether toggling enable_seqscan mid-test is legitimate. Cheers
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
Alvaro Herrera writes: > If this is OK, we should make this API quirkiness very explicit in the > comments, so the patch needs to be a few lines larger in order to be > committable. Also, perhaps the check should be that contype equals > either primary or unique, rather than it doesn't equal foreign. Yeah. See lsyscache.c's get_constraint_index(), as well as commit 641f3dffc which fixed a mighty similar-seeming bug. One question that precedent raises is whether to also include CONSTRAINT_EXCLUSION. But in any case a positive test for the constraint types to allow seems best. regards, tom lane
Re: ICU for global collation
My colleague Andrew Bille found another bug in master
(b4e936859dc441102eb0b6fb7a104f3948c90490) and REL_15_STABLE
(2c63b0930aee1bb5c265fad4a65c9d0b62b1f9da): pg_collation.colliculocale
is not dumped. See check_icu_locale.sh:
In the old cluster:
SELECT collname, colliculocale FROM pg_collation WHERE collname =
'testcoll_backwards'
collname | colliculocale
+---
testcoll_backwards | @colBackwards=yes
(1 row)
In the new cluster:
SELECT collname, colliculocale FROM pg_collation WHERE collname =
'testcoll_backwards'
collname | colliculocale
+---
testcoll_backwards |
(1 row)
diff_dump_colliculocale.patch works for me.
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companyinitdb -D data_old &&
pg_ctl -D data_old -l logfile_old start &&
psql -ac "CREATE COLLATION testcoll_backwards (provider = icu, locale =
'@colBackwards=yes')" postgres &&
echo "In the old cluster:" &&
psql -ac "SELECT collname, colliculocale FROM pg_collation WHERE collname =
'testcoll_backwards'" postgres &&
pg_dump postgres > dump_postgres.sql &&
pg_ctl -D data_old stop &&
initdb -D data_new &&
pg_ctl -D data_new -l logfile_new start &&
psql -v ON_ERROR_STOP=1 -f dump_postgres.sql postgres &&
echo "In the new cluster:" &&
psql -ac "SELECT collname, colliculocale FROM pg_collation WHERE collname =
'testcoll_backwards'" postgres &&
pg_ctl -D data_new stop
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index 2f524b09bf53a55037013d78148f8cbca4fa7eee..9dc5a784dd2d1ce58a6284f19c54730364779c4d 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -17,6 +17,7 @@ top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
export GZIP_PROGRAM=$(GZIP)
+export with_icu
override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS)
LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2c6891573296b395c5bd6b627d061de657355e9c..e8e8f69e30c8a787e5dff157f5d1619917638d7d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -13084,9 +13084,11 @@ dumpCollation(Archive *fout, const CollInfo *collinfo)
int i_collisdeterministic;
int i_collcollate;
int i_collctype;
+ int i_colliculocale;
const char *collprovider;
const char *collcollate;
const char *collctype;
+ const char *colliculocale;
/* Do nothing in data-only dump */
if (dopt->dataOnly)
@@ -13117,6 +13119,13 @@ dumpCollation(Archive *fout, const CollInfo *collinfo)
appendPQExpBufferStr(query,
"true AS collisdeterministic, ");
+ if (fout->remoteVersion >= 15)
+ appendPQExpBufferStr(query,
+ "colliculocale, ");
+ else
+ appendPQExpBufferStr(query,
+ "NULL AS colliculocale, ");
+
appendPQExpBuffer(query,
"collcollate, "
"collctype "
@@ -13130,10 +13139,24 @@ dumpCollation(Archive *fout, const CollInfo *collinfo)
i_collisdeterministic = PQfnumber(res, "collisdeterministic");
i_collcollate = PQfnumber(res, "collcollate");
i_collctype = PQfnumber(res, "collctype");
+ i_colliculocale = PQfnumber(res, "colliculocale");
collprovider = PQgetvalue(res, 0, i_collprovider);
- collcollate = PQgetvalue(res, 0, i_collcollate);
- collctype = PQgetvalue(res, 0, i_collctype);
+
+ if (!PQgetisnull(res, 0, i_collcollate))
+ collcollate = PQgetvalue(res, 0, i_collcollate);
+ else
+ collcollate = NULL;
+
+ if (!PQgetisnull(res, 0, i_collctype))
+ collctype = PQgetvalue(res, 0, i_collctype);
+ else
+ collctype = NULL;
+
+ if (!PQgetisnull(res, 0, i_colliculocale))
+ colliculocale = PQgetvalue(res, 0, i_colliculocale);
+ else
+ colliculocale = NULL;
appendPQExpBuffer(delq, "DROP COLLATION %s;\n",
fmtQualifiedDumpable(collinfo));
@@ -13156,17 +13179,28 @@ dumpCollation(Archive *fout, const CollInfo *collinfo)
if (strcmp(PQgetvalue(res, 0, i_collisdeterministic), "f") == 0)
appendPQExpBufferStr(q, ", deterministic = false");
- if (strcmp(collcollate, collctype) == 0)
+ if (colliculocale != NULL)
{
appendPQExpBufferStr(q, ", locale = ");
- appendStringLiteralAH(q, collcollate, fout);
+ appendStringLiteralAH(q, colliculocale, fout);
}
else
{
- appendPQExpBufferStr(q, ", lc_collate = ");
- appendStringLiteralAH(q, collcollate, fout);
- appendPQExpBufferStr(q, ", lc_ctype = ");
- appendStringLiteralAH(q, collctype, fout);
+ Assert(collcollate != NULL);
+ Assert(collctype != NULL);
+
+ if (strcmp(collcollate, collctype) == 0)
+ {
+ appendPQExpBufferStr(q, ", locale = ");
+ appendStringLiteralAH(q, collcollate, fout);
+ }
+ else
+ {
+ appendPQExpBufferStr(q, ", lc_collate = ");
+ appendStringLiteralAH(q, collcollate, fout);
+ appendPQExpBufferStr(q, ", lc_ctype = ");
+ appendStringLiteralAH(q, collctype, fout);
+ }
}
/*
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl
Re: handling multiple matching constraints in DetachPartitionFinalize()
On 2022-Aug-23, Zhihong Yu wrote: > This is what I came up with. I suggest you provide a set of SQL commands that provoke some wrong behavior with the original code, and show that they generate good behavior after the patch. Otherwise, it's hard to evaluate the usefulness of this. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Aug-23, Zhihong Yu wrote: > A bigger question I have, even with the additional filtering, is what if > there are multiple constraints ? > How do we decide which unique / primary key constraint to return ? > > Looks like there is no known SQL statements leading to such state, but > should we consider such possibility ? I don't think we care, but feel free to experiment and report any problems. You should be able to have multiple UNIQUE constraints on the same column, for example. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Postgres is bloatware by design: it was built to house PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)
Re: add checkpoint stats of snapshot and mapping files of pg_logical dir
On Wed, Aug 17, 2022 at 11:17:24AM +0530, Bharath Rupireddy wrote: > + "logical decoding file(s) > processing time=%ld.%03d s", I would suggest shortening this to something like "logical decoding processing" or "logical replication processing." > CheckPointRelationMap(); > CheckPointReplicationSlots(); > + > + CheckpointStats.l_dec_ops_start_t = GetCurrentTimestamp(); > CheckPointSnapBuild(); > CheckPointLogicalRewriteHeap(); > + CheckpointStats.l_dec_ops_end_t = GetCurrentTimestamp(); > + > CheckPointReplicationOrigin(); Shouldn't we include CheckPointReplicationSlots() and CheckPointReplicationOrigin() in this new stat? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: archive modules
Nathan Bossart writes: > I don't know if it makes sense to document this in basic_archive. On one > hand, it seems like folks will commonly encounter this behavior with this > module, so this feels like a natural place for such a note. But on the > other hand, this is generic behavior for any library that is dynamically > loaded in a separate process. Yeah, I don't think this material is at all specific to basic_archive. Maybe it could be documented near the relevant views, if it isn't already. Also, I think the proposed text neglects the case of including the module in shared_preload_libraries. regards, tom lane
Re: SQL/JSON features for v15
Pavel Stehule writes: > út 23. 8. 2022 v 19:27 odesílatel Andres Freund napsal: >> I think that's not true unfortunately. You can specify return types, and >> composite types can be populated. Which essentially requires arbitrary >> coercions. > Please, can you send an example? Maybe we try to fix a feature that is not > required by standard. Even if it is required by spec, I'd have zero hesitation about tossing that case overboard if that's what we need to do to get to a shippable feature. regards, tom lane
Re: archive modules
On Tue, Aug 23, 2022 at 04:18:52PM +0200, talk to ben wrote: > --- a/doc/src/sgml/basic-archive.sgml > +++ b/doc/src/sgml/basic-archive.sgml > @@ -68,6 +68,19 @@ basic_archive.archive_directory = > '/path/to/archive/directory' > to any archiving still in progress, but users should use extra caution > when > doing so. > > + > + > + The archive module is loaded by the archiver process. Therefore, the > + parameters defined in the module are not set outside this process and > cannot > + be seen from the pg_settings view or the > + \dconfig meta-command. > + These parameters values can be shown from the server's configuration > + file(s) through the pg_file_settings view. > + If you want to check the actual values applied by the archiver, you can > + LOAD the module before reading > + pg_settings. It's also possible to search > + for the options directly with the SHOW command. > + I don't know if it makes sense to document this in basic_archive. On one hand, it seems like folks will commonly encounter this behavior with this module, so this feels like a natural place for such a note. But on the other hand, this is generic behavior for any library that is dynamically loaded in a separate process. Overall, I think I'm +1 for this patch. I haven't thought too much about the exact wording, but provided others support it as well, I will try to take a deeper look soon. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SQL/JSON features for v15
út 23. 8. 2022 v 19:27 odesílatel Andres Freund napsal: > Hi, > > On 2022-08-23 18:06:22 +0200, Pavel Stehule wrote: > > The errors that should be handled are related to json structure errors. I > > don't think so we have to handle all errors and all conversions. > > > > The JSON knows only three types - and these conversions can be written > > specially for this case - or we can write json io routines to be able to > > signal error > > without an exception. > > I think that's not true unfortunately. You can specify return types, and > composite types can be populated. Which essentially requires arbitrary > coercions. > Please, can you send an example? Maybe we try to fix a feature that is not required by standard. Regards Pavel > > Greetings, > > Andres Freund >
Re: SQL/JSON features for v15
On Tue, Aug 23, 2022 at 1:23 PM Andres Freund wrote: > > But that's exactly what I'm complaining about. Catching an error that > > unwound a bunch of stack frames where complicated things are happening > > is fraught with peril. There's probably a bunch of errors that could > > be thrown from somewhere in that code - out of memory being a great > > example - that should not be caught. > > The code as is handles this to some degree. Only ERRCODE_DATA_EXCEPTION, > ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION are caught, the rest is immediately > rethrown. AFAIK, Tom has rejected every previous effort to introduce this type of coding into the tree rather forcefully. What makes it OK now? > I'm not sure what the general alternative is though. Part of the feature is > generating a composite type from json - there's just no way we can make all > possible coercion pathways not error out. That'd necessitate requiring all > builtin types and extensions types out there to provide input functions that > don't throw on invalid input and all coercions to not throw either. That just > seems unrealistic. Well, I think that having input functions report input that is not valid for the data type in some way other than just chucking an error as they'd also do for a missing TOAST chunk would be a pretty sensible plan. I'd support doing that if we forced a hard compatibility break, and I'd support that if we provided some way for old code to continue running in degraded mode. I haven't thought too much about the coercion case, but I suppose the issues are similar. What I don't support is saying -- well, upgrading our infrastructure is hard, so let's just kludge it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: SQL/JSON features for v15
Andres Freund writes: > On 2022-08-23 12:26:55 -0400, Robert Haas wrote: >> But that's exactly what I'm complaining about. Catching an error that >> unwound a bunch of stack frames where complicated things are happening >> is fraught with peril. There's probably a bunch of errors that could >> be thrown from somewhere in that code - out of memory being a great >> example - that should not be caught. > The code as is handles this to some degree. Only ERRCODE_DATA_EXCEPTION, > ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION are caught, the rest is immediately > rethrown. That's still a lot of territory, considering how nonspecific most SQLSTATEs are. Even if you can prove that only the intended cases are caught today, somebody could inadvertently break it next week by using one of those codes somewhere else. I agree with the upthread comments that we only need/want to catch foreseeable incorrect-input errors, and that the way to make that happen is to refactor the related type input functions, and that a lot of the heavy lifting for that has been done already. regards, tom lane
Re: SQL/JSON features for v15
Hi, On 2022-08-23 18:06:22 +0200, Pavel Stehule wrote: > The errors that should be handled are related to json structure errors. I > don't think so we have to handle all errors and all conversions. > > The JSON knows only three types - and these conversions can be written > specially for this case - or we can write json io routines to be able to > signal error > without an exception. I think that's not true unfortunately. You can specify return types, and composite types can be populated. Which essentially requires arbitrary coercions. Greetings, Andres Freund
Re: SQL/JSON features for v15
Hi, On 2022-08-23 13:18:49 -0400, Jonathan S. Katz wrote: > Taking RMT hat off, if the outcome is "revert", I do want to ensure we don't > lose momentum on getting this into v16. I know a lot of time and effort has > gone into this featureset and it seems to be trending in the right > direction. We have a mixed history on reverts in terms of if/when they are > committed and I don't want to see that happen to these features. I do think > this will remain a headline feature even if we delay it for v16. We could decide to revert this for 15, but leave it in tree for HEAD. Greetings, Andres Freund
Re: SQL/JSON features for v15
"Jonathan S. Katz" writes: > I saw Andrew suggest that the controversial parts of the patchset may be > severable from some of the new functionality, so I would like to see > that proposal and if it is enough to overcome concerns. It's an interesting suggestion. Do people have the cycles available to make it happen in the next few days? regards, tom lane
Re: SQL/JSON features for v15
Hi, On 2022-08-23 12:26:55 -0400, Robert Haas wrote: > On Tue, Aug 23, 2022 at 11:55 AM Andres Freund wrote: > > I don't think that's quite realistic - that's the input/output functions for > > all types, basically. I'd be somewhat content if we'd a small list of very > > common coercion paths we knew wouldn't error out, leaving things like OOM > > aside. Even just knowing that for ->text conversions would be a huge deal in > > the context of this patch. One problem here is that the whole type coercion > > infrastructure doesn't make it easy to know what "happened inside" atm, one > > has to reconstruct it from the emitted expressions, where there can be > > multiple layers of things to poke through. > > But that's exactly what I'm complaining about. Catching an error that > unwound a bunch of stack frames where complicated things are happening > is fraught with peril. There's probably a bunch of errors that could > be thrown from somewhere in that code - out of memory being a great > example - that should not be caught. The code as is handles this to some degree. Only ERRCODE_DATA_EXCEPTION, ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION are caught, the rest is immediately rethrown. > What you (probably) want is to know whether one specific error happened or > not, and catch only that one. And the error machinery isn't designed for > that. It's not designed to let you catch specific errors for specific call > sites, and it's also not designed to be particularly efficient if lots of > errors need to be caught over and over again. If you decide to ignore all > that and do it anyway, you'll end up with, at best, code that is > complicated, hard to maintain, and probably slow when a lot of errors are > trapped, and at worst, code that is fragile or outright buggy. I'm not sure what the general alternative is though. Part of the feature is generating a composite type from json - there's just no way we can make all possible coercion pathways not error out. That'd necessitate requiring all builtin types and extensions types out there to provide input functions that don't throw on invalid input and all coercions to not throw either. That just seems unrealistic. I think the best we could without subtransactions do perhaps is to add metadata to pg_cast, pg_type telling us whether certain types of errors are possible, and requiring ERROR ON ERROR when coercion paths are required that don't have those options. Greetings, Andres Freund
Re: handling multiple matching constraints in DetachPartitionFinalize()
On Tue, Aug 23, 2022 at 10:10 AM Zhihong Yu wrote: > Hi, > I was looking at the following code in DetachPartitionFinalize(): > > /* If there's a constraint associated with the index, detach it > too */ > constrOid = > get_relation_idx_constraint_oid(RelationGetRelid(partRel), > idxid); > > As mentioned in email thread `parenting a PK constraint to a self-FK one`, > there may be multiple matching constraints, I think we should > call ConstraintSetParentConstraint() for each of them. > > This means adding a helper method similar to > get_relation_idx_constraint_oid() which finds constraint and calls > ConstraintSetParentConstraint(). > > I am preparing a patch. > Please let me know if my proposal makes sense. > > Thanks > This is what I came up with. detach-constraints.patch Description: Binary data
Re: SQL/JSON features for v15
On 8/23/22 11:08 AM, Tom Lane wrote: Robert Haas writes: At the end of the day, the RMT is going to have to take a call here. It seems to me that Andres's concerns about code quality and lack of comments are probably somewhat legitimate, and in particular I do not think the use of subtransactions is a good idea. I also don't think that trying to fix those problems or generally improve the code by committing thousands of lines of new code in August when we're targeting a release in September or October is necessarily a good idea. But I'm also not in a position to say that the project is going to be irreparably damaged if we just ship what we've got, perhaps after fixing the most acute problems that we currently know about. The problem here is that this was going to be a headline new feature for v15. Shipping what apparently is only an alpha-quality implementation seems pretty problematic unless we advertise it as such, and that's not something we've done very much in the past. With my user hat on, we have done this before -- if inadvertently -- but agree it's not recommended nor a habit we should get into. As you say, we've delegated this sort of decision to the RMT, but if I were on the RMT I'd be voting to revert. With RMT hat on,the RMT does have power of forced commit/revert in absence of consensus through regular community processes[1]. We did explicitly discuss at our meeting today if we were going to make the decision right now. We decided that we would come back and set a deadline on letting the community processes play out, otherwise we will make the decision. For decision deadline: if there is no community consensus by end of Aug 28, 2022 AoE, the RMT will make the decision. I know Andrew has been prepping for the outcome of a revert -- this should give enough for review and merge prior to a Beta 4 release (targeted for Sep 8). If there is concern about that, the RMT can move up the decision timeframe. Taking RMT hat off, if the outcome is "revert", I do want to ensure we don't lose momentum on getting this into v16. I know a lot of time and effort has gone into this featureset and it seems to be trending in the right direction. We have a mixed history on reverts in terms of if/when they are committed and I don't want to see that happen to these features. I do think this will remain a headline feature even if we delay it for v16. I saw Andrew suggest that the controversial parts of the patchset may be severable from some of the new functionality, so I would like to see that proposal and if it is enough to overcome concerns. Thanks, Jonathan [1] https://wiki.postgresql.org/wiki/Release_Management_Team OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH] Optimize json_lex_string by batching character copying
On Tue, Aug 23, 2022 at 01:03:03PM +0700, John Naylor wrote: > On Tue, Aug 23, 2022 at 10:32 AM Nathan Bossart >> Here's a new version of the patch with the 32-bit changes and calls to >> lfind() removed. > > LGTM overall. My plan is to split out the json piece, adding tests for > that, and commit the infrastructure for it fairly soon. Possible > bikeshedding: Functions like vector8_eq() might be misunderstood as > comparing two vectors, but here we are comparing each lane with a > scalar. I wonder if vector8_eq_scalar() et al might be more clear. Good point. I had used vector32_veq() to denote vector comparison, which would extend to something like vector8_seq(). But that doesn't seem descriptive enough. It might be worth considering vector8_contains() or vector8_has() as well. I don't really have an opinion, but if I had to pick something, I guess I'd choose vector8_contains(). -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: [PATCH] Expose port->authn_id to extensions and triggers
On 8/23/22 01:53, Drouvot, Bertrand wrote: > That sounds all good to me, except a typo for the author in the commit > message: s/Jocob/Jacob/ Thanks, I missed that on my readthrough! :D Patch looks good to me, too, with one question: > @@ -2688,6 +2689,7 @@ InitProcessGlobals(void) > > MyProcPid = getpid(); > > MyStartTimestamp = GetCurrentTimestamp(); > > MyStartTime = timestamptz_to_time_t(MyStartTimestamp); > > + memset(, 0, sizeof(MyClientConnectionInfo)); > > > > /* > > * Set a different global seed in every process. We want something When can we rely on static initialization, and when can't we? Is there a concern that the memory could have been polluted from before the postmaster's fork? Thanks, --Jacob
handling multiple matching constraints in DetachPartitionFinalize()
Hi, I was looking at the following code in DetachPartitionFinalize(): /* If there's a constraint associated with the index, detach it too */ constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel), idxid); As mentioned in email thread `parenting a PK constraint to a self-FK one`, there may be multiple matching constraints, I think we should call ConstraintSetParentConstraint() for each of them. This means adding a helper method similar to get_relation_idx_constraint_oid() which finds constraint and calls ConstraintSetParentConstraint(). I am preparing a patch. Please let me know if my proposal makes sense. Thanks
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Tue, Aug 23, 2022 at 9:47 AM Alvaro Herrera
wrote:
> On 2022-Aug-23, Zhihong Yu wrote:
>
> > I was thinking of the following patch.
> > Basically, if there is only one matching constraint. we still return it.
> >
> > diff --git a/src/postgres/src/backend/catalog/pg_constraint.c
> > b/src/postgres/src/backend/catalog/pg_constraint.c
> > index f0726e9aa0..ddade138b4 100644
> > --- a/src/postgres/src/backend/catalog/pg_constraint.c
> > +++ b/src/postgres/src/backend/catalog/pg_constraint.c
> > @@ -1003,7 +1003,8 @@ get_relation_idx_constraint_oid(Oid relationId, Oid
> > indexId)
> > constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
> > if (constrForm->conindid == indexId)
> > {
> > - constraintId = HeapTupleGetOid(tuple);
> > + if (constraintId == InvalidOid || constrForm->confrelid == 0)
> > + constraintId = HeapTupleGetOid(tuple);
> > break;
> > }
> > }
>
> We could do this, but what do we gain by doing so? It seems to me that
> my proposed formulation achieves the same and is less fuzzy about what
> the returned constraint is. Please try to write a code comment that
> explains what this does and see if it makes sense.
>
> For my proposal, it would be "return the OID of a primary key or unique
> constraint associated with the given index in the given relation, or OID
> if no such index is catalogued". This definition is clearly useful for
> partitioned tables, on which the unique and primary key constraints are
> useful elements. There's nothing that cares about foreign keys.
>
> --
> Álvaro Herrera PostgreSQL Developer —
> https://www.EnterpriseDB.com/
> "La virtud es el justo medio entre dos defectos" (Aristóteles)
>
A bigger question I have, even with the additional filtering, is what if
there are multiple constraints ?
How do we decide which unique / primary key constraint to return ?
Looks like there is no known SQL statements leading to such state, but
should we consider such possibility ?
Cheers
Re: cataloguing NOT NULL constraints
On 2022-Aug-22, Amit Langote wrote: > Yeah, I think it makes sense to think of the NOT NULL constraints on > their own in this case, without worrying about the PK constraint that > created them in the first place. Cool, that's enough votes that I'm comfortable implementing things that way. > BTW, maybe you are aware, but the legacy inheritance implementation is > not very consistent about wanting to maintain the same NULLness for a > given column in all members of the inheritance tree. For example, it > allows one to alter the NULLness of an inherited column: Right ... I think what gives this patch most of its complexity is the number of odd, inconsistent cases that have to preserve historical behavior. Luckily I think this particular behavior is easy to implement. > IIRC, I had tried to propose implementing the same behavior for legacy > inheritance back in the day, but maybe we left it alone for not > breaking compatibility. Yeah, that wouldn't be surprising. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "The problem with the facetime model is not just that it's demoralizing, but that the people pretending to work interrupt the ones actually working." (Paul Graham)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Aug-23, Zhihong Yu wrote:
> I was thinking of the following patch.
> Basically, if there is only one matching constraint. we still return it.
>
> diff --git a/src/postgres/src/backend/catalog/pg_constraint.c
> b/src/postgres/src/backend/catalog/pg_constraint.c
> index f0726e9aa0..ddade138b4 100644
> --- a/src/postgres/src/backend/catalog/pg_constraint.c
> +++ b/src/postgres/src/backend/catalog/pg_constraint.c
> @@ -1003,7 +1003,8 @@ get_relation_idx_constraint_oid(Oid relationId, Oid
> indexId)
> constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
> if (constrForm->conindid == indexId)
> {
> - constraintId = HeapTupleGetOid(tuple);
> + if (constraintId == InvalidOid || constrForm->confrelid == 0)
> + constraintId = HeapTupleGetOid(tuple);
> break;
> }
> }
We could do this, but what do we gain by doing so? It seems to me that
my proposed formulation achieves the same and is less fuzzy about what
the returned constraint is. Please try to write a code comment that
explains what this does and see if it makes sense.
For my proposal, it would be "return the OID of a primary key or unique
constraint associated with the given index in the given relation, or OID
if no such index is catalogued". This definition is clearly useful for
partitioned tables, on which the unique and primary key constraints are
useful elements. There's nothing that cares about foreign keys.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)
Re: SQL/JSON features for v15
On 23.08.2022 19:06, Pavel Stehule wrote: Hi út 23. 8. 2022 v 17:55 odesílatel Andres Freund napsal: Hi, On 2022-08-23 10:51:04 -0400, Robert Haas wrote: > I do not think that using subtransactions as part of the expression > evaluation process is a sound idea pretty much under any > circumstances. Maybe if the subtransations aren't commonly created and > don't usually get XIDs there wouldn't be a big problem in practice, > but it's an awfully heavyweight operation to be done inside expression > evaluation even in corner cases. I think that if we need to make > certain operations that would throw errors not throw errors, we need > to refactor interfaces until it's possible to return an error > indicator up to the appropriate level, not just let the error be > thrown and catch it. I don't think that's quite realistic - that's the input/output functions for all types, basically. I'd be somewhat content if we'd a small list of very common coercion paths we knew wouldn't error out, leaving things like OOM aside. Even just knowing that for ->text conversions would be a huge deal in the context of this patch. One problem here is that the whole type coercion infrastructure doesn't make it easy to know what "happened inside" atm, one has to reconstruct it from the emitted expressions, where there can be multiple layers of things to poke through. The errors that should be handled are related to json structure errors. I don't think so we have to handle all errors and all conversions. The JSON knows only three types - and these conversions can be written specially for this case - or we can write json io routines to be able to signal error without an exception. I also wanted to suggest to limit the set of returning types to the predefined set of JSON-compatible types for which can write safe conversion functions: character types (text, char), boolean, number types (integers, floats types, numeric), datetime types. The SQL standard even does not require support of other returning types. For the float8 and datetime types we already have safe input functions like float8in_internal_opt_error() and parse_datetime() which are used inside jsonpath and return error code instead of throwing errors. We need to implement numeric_intN_safe() and maybe a few other trivial functions like that. The set of returning types, for which we do not need any special coercions, is very limited: json, jsonb, text. More precisely, even RETURNING json[b] can throw errors in JSON_QUERY(OMIT QUOTES), and we also need safe json parsing, but it can be easily done with pg_parse_json(), which returns error code.
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Tue, Aug 23, 2022 at 9:30 AM Alvaro Herrera
wrote:
> On 2022-Aug-23, Jehan-Guillaume de Rorthais wrote:
>
> Hi,
>
> [...]
>
> > However, it seems get_relation_idx_constraint_oid(), introduced in
> eb7ed3f3063,
> > assume there could be only ONE constraint depending to an index. But in
> fact,
> > multiple constraints can rely on the same index, eg.: the PK and a self
> > referencing FK. In consequence, when looking for a constraint depending
> on an
> > index for the given relation, either the FK or a PK can appears first
> depending
> > on various conditions. It is then possible to trick it make a FK
> constraint a
> > parent of a PK...
>
> Hmm, wow, that sounds extremely stupid. I think a sufficient fix might
> be to have get_relation_idx_constraint_oid ignore any constraints that
> are not unique or primary keys. I tried your scenario with the attached
> and it seems to work correctly. Can you confirm? (I only ran the
> pg_regress tests, not anything else for now.)
>
> If this is OK, we should make this API quirkiness very explicit in the
> comments, so the patch needs to be a few lines larger in order to be
> committable. Also, perhaps the check should be that contype equals
> either primary or unique, rather than it doesn't equal foreign.
>
> --
> Álvaro Herrera 48°01'N 7°57'E —
> https://www.EnterpriseDB.com/
I was thinking of the following patch.
Basically, if there is only one matching constraint. we still return it.
diff --git a/src/postgres/src/backend/catalog/pg_constraint.c
b/src/postgres/src/backend/catalog/pg_constraint.c
index f0726e9aa0..ddade138b4 100644
--- a/src/postgres/src/backend/catalog/pg_constraint.c
+++ b/src/postgres/src/backend/catalog/pg_constraint.c
@@ -1003,7 +1003,8 @@ get_relation_idx_constraint_oid(Oid relationId, Oid
indexId)
constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
if (constrForm->conindid == indexId)
{
- constraintId = HeapTupleGetOid(tuple);
+ if (constraintId == InvalidOid || constrForm->confrelid == 0)
+ constraintId = HeapTupleGetOid(tuple);
break;
}
}
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Aug-23, Jehan-Guillaume de Rorthais wrote:
Hi,
[...]
> However, it seems get_relation_idx_constraint_oid(), introduced in
> eb7ed3f3063,
> assume there could be only ONE constraint depending to an index. But in fact,
> multiple constraints can rely on the same index, eg.: the PK and a self
> referencing FK. In consequence, when looking for a constraint depending on an
> index for the given relation, either the FK or a PK can appears first
> depending
> on various conditions. It is then possible to trick it make a FK constraint a
> parent of a PK...
Hmm, wow, that sounds extremely stupid. I think a sufficient fix might
be to have get_relation_idx_constraint_oid ignore any constraints that
are not unique or primary keys. I tried your scenario with the attached
and it seems to work correctly. Can you confirm? (I only ran the
pg_regress tests, not anything else for now.)
If this is OK, we should make this API quirkiness very explicit in the
comments, so the patch needs to be a few lines larger in order to be
committable. Also, perhaps the check should be that contype equals
either primary or unique, rather than it doesn't equal foreign.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
>From a39299db23f4b2495b3ba10e7adb1121f0271694 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 23 Aug 2022 18:17:24 +0200
Subject: [PATCH] test fix
---
src/backend/catalog/pg_constraint.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index bb65fb1e0a..71b6fb35e6 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -1011,6 +1011,14 @@ get_relation_idx_constraint_oid(Oid relationId, Oid indexId)
Form_pg_constraint constrForm;
constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
+
+ /*
+ * Foreign key constraints are ignored for these purposes,
+ * because ...?
+ */
+ if (constrForm->contype == CONSTRAINT_FOREIGN)
+ continue;
+
if (constrForm->conindid == indexId)
{
constraintId = constrForm->oid;
--
2.30.2
Re: CFM Manager
On Mon, Aug 22, 2022 at 1:50 PM Ibrar Ahmed wrote: > This will help to complete the tasks. I start looking at that; I will let you > know how we both > manage to share the load I have updated the CFM checklist through the "2 days before CF" section. Let me know if you have questions/suggestions. Thanks, --Jacob
Re: SQL/JSON features for v15
On Tue, Aug 23, 2022 at 11:55 AM Andres Freund wrote: > I don't think that's quite realistic - that's the input/output functions for > all types, basically. I'd be somewhat content if we'd a small list of very > common coercion paths we knew wouldn't error out, leaving things like OOM > aside. Even just knowing that for ->text conversions would be a huge deal in > the context of this patch. One problem here is that the whole type coercion > infrastructure doesn't make it easy to know what "happened inside" atm, one > has to reconstruct it from the emitted expressions, where there can be > multiple layers of things to poke through. But that's exactly what I'm complaining about. Catching an error that unwound a bunch of stack frames where complicated things are happening is fraught with peril. There's probably a bunch of errors that could be thrown from somewhere in that code - out of memory being a great example - that should not be caught. What you (probably) want is to know whether one specific error happened or not, and catch only that one. And the error machinery isn't designed for that. It's not designed to let you catch specific errors for specific call sites, and it's also not designed to be particularly efficient if lots of errors need to be caught over and over again. If you decide to ignore all that and do it anyway, you'll end up with, at best, code that is complicated, hard to maintain, and probably slow when a lot of errors are trapped, and at worst, code that is fragile or outright buggy. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
Hi,
Thanks for the review!
>
> 1.
> In FilterOutNotSuitablePathsForReplIdentFull(), is
> "nonPartialIndexPathList" a
> good name for the list? Indexes on only expressions are also be filtered.
>
> +static List *
> +FilterOutNotSuitablePathsForReplIdentFull(List *pathlist)
> +{
> + ListCell *lc;
> + List *nonPartialIndexPathList = NIL;
>
>
Yes, true. We only started filtering the non-partial ones first. Now
changed to *suitableIndexList*, does that look right?
> 2.
> +typedef struct LogicalRepPartMapEntry
> +{
> + Oid partoid;/*
> LogicalRepPartMap's key */
> + LogicalRepRelMapEntry relmapentry;
> + Oid usableIndexOid; /* which index to use?
> (Invalid when no index
> +* used) */
> +} LogicalRepPartMapEntry;
>
> For partition tables, is it possible to use relmapentry->usableIndexOid to
> mark
> which index to use? Which means we don't need to add "usableIndexOid" to
> LogicalRepPartMapEntry.
>
>
My intention was to make this explicit so that it is clear that partitions
can explicitly own indexes.
But I tried your suggested refactor, which looks good. So, I changed it.
Also, I realized that I do not have a test where the partition has an index
(not inherited from the parent), which I also added now.
> 3.
> It looks we should change the comment for FindReplTupleInLocalRel() in this
> patch.
>
> /*
> * Try to find a tuple received from the publication side (in
> 'remoteslot') in
> * the corresponding local relation using either replica identity index,
> * primary key or if needed, sequential scan.
> *
> * Local tuple, if found, is returned in '*localslot'.
> */
> static bool
> FindReplTupleInLocalRel(EState *estate, Relation localrel,
>
>
I made a small change, just adding "index". Do you expect a larger change?
> 4.
> @@ -2030,16 +2017,19 @@ apply_handle_delete_internal(ApplyExecutionData
> *edata,
> {
> EState *estate = edata->estate;
> Relationlocalrel = relinfo->ri_RelationDesc;
> - LogicalRepRelation *remoterel = >targetRel->remoterel;
> + LogicalRepRelMapEntry *targetRel = edata->targetRel;
> + LogicalRepRelation *remoterel = >remoterel;
> EPQStateepqstate;
> TupleTableSlot *localslot;
>
> Do we need this change? I didn't see any place to use the variable
> targetRel
> afterwards.
>
Seems so, changed it back.
> 5.
> + if (!AttributeNumberIsValid(mainattno))
> + {
> + /*
> +* There are two cases to consider. First, if the
> index is a primary or
> +* unique key, we cannot have any indexes with
> expressions. So, at this
> +* point we are sure that the index we deal is not
> these.
> +*/
> + Assert(RelationGetReplicaIndex(rel) !=
> RelationGetRelid(idxrel) &&
> + RelationGetPrimaryKeyIndex(rel) !=
> RelationGetRelid(idxrel));
> +
> + /*
> +* For a non-primary/unique index with an
> expression, we are sure that
> +* the expression cannot be used for replication
> index search. The
> +* reason is that we create relevant index paths
> by providing column
> +* equalities. And, the planner does not pick
> expression indexes via
> +* column equality restrictions in the query.
> +*/
> + continue;
> + }
>
> Is it possible that it is a usable index with an expression? I think
> indexes
> with an expression has been filtered in
> FilterOutNotSuitablePathsForReplIdentFull(). If it can't be a usable index
> with
> an expression, maybe we shouldn't use "continue" here.
>
Ok, I think there are some confusing comments in the code, which I updated.
Also, added one more explicit Assert to make the code a little more
readable.
We can support indexes involving expressions but not indexes that are only
consisting of expressions. FilterOutNotSuitablePathsForReplIdentFull()
filters out the latter, see IndexOnlyOnExpression().
So, for example, if we have an index as below, we are skipping the
expression while building the index scan keys:
CREATE INDEX people_names ON people (firstname, lastname, (id || '_' ||
sub_id));
We can consider removing `continue`, but that'd mean we should also adjust
the following code-block to handle indexprs. To me, that seems like an edge
case to implement at this point, given such an index is probably not
common. Do you think should I try to use the indexprs as well while
building the scan key?
I'm mostly trying to keep the complexity small. If you suggest this
limitation should be lifted, I can give it a shot. I think the limitation I
leave here is with a
Re: SQL/JSON features for v15
Hi út 23. 8. 2022 v 17:55 odesílatel Andres Freund napsal: > Hi, > > On 2022-08-23 10:51:04 -0400, Robert Haas wrote: > > I do not think that using subtransactions as part of the expression > > evaluation process is a sound idea pretty much under any > > circumstances. Maybe if the subtransations aren't commonly created and > > don't usually get XIDs there wouldn't be a big problem in practice, > > but it's an awfully heavyweight operation to be done inside expression > > evaluation even in corner cases. I think that if we need to make > > certain operations that would throw errors not throw errors, we need > > to refactor interfaces until it's possible to return an error > > indicator up to the appropriate level, not just let the error be > > thrown and catch it. > > I don't think that's quite realistic - that's the input/output functions > for > all types, basically. I'd be somewhat content if we'd a small list of very > common coercion paths we knew wouldn't error out, leaving things like OOM > aside. Even just knowing that for ->text conversions would be a huge deal > in > the context of this patch. One problem here is that the whole type > coercion > infrastructure doesn't make it easy to know what "happened inside" atm, one > has to reconstruct it from the emitted expressions, where there can be > multiple layers of things to poke through. > The errors that should be handled are related to json structure errors. I don't think so we have to handle all errors and all conversions. The JSON knows only three types - and these conversions can be written specially for this case - or we can write json io routines to be able to signal error without an exception. Regards Pavel > > Greetings, > > Andres Freund > > >
Re: SQL/JSON features for v15
Hi, On 2022-08-23 10:51:04 -0400, Robert Haas wrote: > I do not think that using subtransactions as part of the expression > evaluation process is a sound idea pretty much under any > circumstances. Maybe if the subtransations aren't commonly created and > don't usually get XIDs there wouldn't be a big problem in practice, > but it's an awfully heavyweight operation to be done inside expression > evaluation even in corner cases. I think that if we need to make > certain operations that would throw errors not throw errors, we need > to refactor interfaces until it's possible to return an error > indicator up to the appropriate level, not just let the error be > thrown and catch it. I don't think that's quite realistic - that's the input/output functions for all types, basically. I'd be somewhat content if we'd a small list of very common coercion paths we knew wouldn't error out, leaving things like OOM aside. Even just knowing that for ->text conversions would be a huge deal in the context of this patch. One problem here is that the whole type coercion infrastructure doesn't make it easy to know what "happened inside" atm, one has to reconstruct it from the emitted expressions, where there can be multiple layers of things to poke through. Greetings, Andres Freund
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Tue, Aug 23, 2022 at 8:07 AM Jehan-Guillaume de Rorthais
wrote:
> Hi all,
>
> I've been able to work on this issue and isolate where in the code the
> oddity
> is laying.
>
> During ATExecAttachPartition(), AttachPartitionEnsureIndexes() look for
> existing
> required index on the partition to attach. It creates missing index, or
> sets the
> parent's index when a matching one exists on the partition. Good.
>
> When a matching index is found, if the parent index enforce a constraint,
> the
> function look for the similar constraint in the partition-to-be, and set
> the
> constraint parent as well:
>
> constraintOid =
> get_relation_idx_constraint_oid(RelationGetRelid(rel),
> idx);
>
> [...]
>
> /*
> * If this index is being created in the parent because of a
> * constraint, then the child needs to have a constraint also,
> * so look for one. If there is no such constraint, this
> * index is no good, so keep looking.
> */
> if (OidIsValid(constraintOid))
> {
> cldConstrOid = get_relation_idx_constraint_oid(
> RelationGetRelid(attachrel),
> cldIdxId);
> /* no dice */
> if (!OidIsValid(cldConstrOid))
> continue;
> }
> /* bingo. */
> IndexSetParentIndex(attachrelIdxRels[i], idx);
> if (OidIsValid(constraintOid))
> ConstraintSetParentConstraint(cldConstrOid, constraintOid,
> RelationGetRelid(attachrel));
>
> However, it seems get_relation_idx_constraint_oid(), introduced in
> eb7ed3f3063,
> assume there could be only ONE constraint depending to an index. But in
> fact,
> multiple constraints can rely on the same index, eg.: the PK and a self
> referencing FK. In consequence, when looking for a constraint depending on
> an
> index for the given relation, either the FK or a PK can appears first
> depending
> on various conditions. It is then possible to trick it make a FK
> constraint a
> parent of a PK...
>
> In the following little scenario, when looking at the constraint linked to
> the PK unique index using the same index than
> get_relation_idx_constraint_oid
> use, this is the self-FK that is actually returned first by
> get_relation_idx_constraint_oid(), NOT the PK:
>
> postgres=# DROP TABLE IF EXISTS parent, child1;
>
> CREATE TABLE parent (
> id bigint NOT NULL default 1,
> no_part smallint NOT NULL,
> id_abc bigint,
> FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part)
> ON UPDATE RESTRICT ON DELETE RESTRICT,
> PRIMARY KEY (id, no_part)
> )
> PARTITION BY LIST (no_part);
>
> CREATE TABLE child1 (
> id bigint NOT NULL default 1,
> no_part smallint NOT NULL,
> id_abc bigint,
> PRIMARY KEY (id, no_part),
> CONSTRAINT child1 CHECK ((no_part = 1))
> );
>
> -- force an indexscan as get_relation_idx_constraint_oid() use the
> unique
> -- index on (conrelid, contypid, conname) to scan pg_cosntraint
> set enable_seqscan TO off;
> set enable_bitmapscan TO off;
>
> SELECT conname
> FROM pg_constraint
> WHERE conrelid = 'parent'::regclass <=== parent
> AND conindid = 'parent_pkey'::regclass; <=== PK index
>
> DROP TABLE
> CREATE TABLE
> CREATE TABLE
> SET
> SET
> conname
>
>parent_id_abc_no_part_fkey < WOOPS!
>parent_pkey
> (2 rows)
>
> In consequence, when attaching the partition, the PK of child1 is not
> marked as
> partition of the parent's PK, which is wrong. WORST, the PK of child1 is
> actually unexpectedly marked as a partition of the parent's **self-FK**:
>
> postgres=# ALTER TABLE ONLY parent ATTACH PARTITION child1
> FOR VALUES IN ('1');
>
> SELECT oid, conname, conparentid, conrelid, confrelid
> FROM pg_constraint
> WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
> ORDER BY 1;
>
> ALTER TABLE
> oid | conname | conparentid | conrelid |
> confrelid
>
> ---+-+-+--+---
>16700 | parent_pkey | 0 |16695 | 0
>16701 | parent_id_abc_no_part_fkey | 0 |16695 | 16695
>16706 | child1 | 0 |16702 | 0
>16708 | **child1_pkey** | **16701** |16702 | 0
>16709 | parent_id_abc_no_part_fkey1 | 16701 |16695 | 16702
>16712 | parent_id_abc_no_part_fkey | 16701 |16702 | 16695
> (6 rows)
>
> The expected result should probably be something like:
>
> oid | conname | conparentid | conrelid
Re: SQL/JSON features for v15
On 2022-08-23 Tu 11:08, Tom Lane wrote: > Robert Haas writes: >> At the end of the day, the RMT is going to have to take a call here. >> It seems to me that Andres's concerns about code quality and lack of >> comments are probably somewhat legitimate, and in particular I do not >> think the use of subtransactions is a good idea. I also don't think >> that trying to fix those problems or generally improve the code by >> committing thousands of lines of new code in August when we're >> targeting a release in September or October is necessarily a good >> idea. But I'm also not in a position to say that the project is going >> to be irreparably damaged if we just ship what we've got, perhaps >> after fixing the most acute problems that we currently know about. > The problem here is that this was going to be a headline new feature > for v15. Shipping what apparently is only an alpha-quality implementation > seems pretty problematic unless we advertise it as such, and that's > not something we've done very much in the past. I also wonder how > much any attempts at fixing it later would be constrained by concerns > about compatibility with the v15 version. > >> ... And we do have other bad code in the system. > Can't deny that, but a lot of it is legacy code that we wish we could > rip out and can't because backwards compatibility. This is not legacy > code ... not yet anyway. > > As you say, we've delegated this sort of decision to the RMT, but > if I were on the RMT I'd be voting to revert. > > I know I previously said that this was not really severable, but I've started having second thoughts about that. If we disabled as Not Implemented the DEFAULT form of the ON ERROR and ON EMPTY clauses, and possibly the RETURNING clause in some cases, it's possible we could get rid of most of what's been controversial. That could still leave us a good deal of what we want, including JSON_TABLE, which is by far the most interesting of these features. I haven't looked closely yet at how possible this is, it only occurred to me today, but I think it's worth exploring. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: SQL/JSON features for v15
Hi, On 2022-08-23 11:08:31 -0400, Tom Lane wrote: > As you say, we've delegated this sort of decision to the RMT, but > if I were on the RMT I'd be voting to revert. Yea, I don't really see an alternative at this point. If we really wanted we could try to cut the more complicated pieces out, e.g., by only supporting ERROR ON ERROR, but I'm not sure it'd get us far enough. Greetings, Andres Freund
Re: SQL/JSON features for v15
Robert Haas writes: > At the end of the day, the RMT is going to have to take a call here. > It seems to me that Andres's concerns about code quality and lack of > comments are probably somewhat legitimate, and in particular I do not > think the use of subtransactions is a good idea. I also don't think > that trying to fix those problems or generally improve the code by > committing thousands of lines of new code in August when we're > targeting a release in September or October is necessarily a good > idea. But I'm also not in a position to say that the project is going > to be irreparably damaged if we just ship what we've got, perhaps > after fixing the most acute problems that we currently know about. The problem here is that this was going to be a headline new feature for v15. Shipping what apparently is only an alpha-quality implementation seems pretty problematic unless we advertise it as such, and that's not something we've done very much in the past. I also wonder how much any attempts at fixing it later would be constrained by concerns about compatibility with the v15 version. > ... And we do have other bad code in the system. Can't deny that, but a lot of it is legacy code that we wish we could rip out and can't because backwards compatibility. This is not legacy code ... not yet anyway. As you say, we've delegated this sort of decision to the RMT, but if I were on the RMT I'd be voting to revert. regards, tom lane
[BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
Hi all,
I've been able to work on this issue and isolate where in the code the oddity
is laying.
During ATExecAttachPartition(), AttachPartitionEnsureIndexes() look for existing
required index on the partition to attach. It creates missing index, or sets the
parent's index when a matching one exists on the partition. Good.
When a matching index is found, if the parent index enforce a constraint, the
function look for the similar constraint in the partition-to-be, and set the
constraint parent as well:
constraintOid = get_relation_idx_constraint_oid(RelationGetRelid(rel),
idx);
[...]
/*
* If this index is being created in the parent because of a
* constraint, then the child needs to have a constraint also,
* so look for one. If there is no such constraint, this
* index is no good, so keep looking.
*/
if (OidIsValid(constraintOid))
{
cldConstrOid = get_relation_idx_constraint_oid(
RelationGetRelid(attachrel),
cldIdxId);
/* no dice */
if (!OidIsValid(cldConstrOid))
continue;
}
/* bingo. */
IndexSetParentIndex(attachrelIdxRels[i], idx);
if (OidIsValid(constraintOid))
ConstraintSetParentConstraint(cldConstrOid, constraintOid,
RelationGetRelid(attachrel));
However, it seems get_relation_idx_constraint_oid(), introduced in eb7ed3f3063,
assume there could be only ONE constraint depending to an index. But in fact,
multiple constraints can rely on the same index, eg.: the PK and a self
referencing FK. In consequence, when looking for a constraint depending on an
index for the given relation, either the FK or a PK can appears first depending
on various conditions. It is then possible to trick it make a FK constraint a
parent of a PK...
In the following little scenario, when looking at the constraint linked to
the PK unique index using the same index than get_relation_idx_constraint_oid
use, this is the self-FK that is actually returned first by
get_relation_idx_constraint_oid(), NOT the PK:
postgres=# DROP TABLE IF EXISTS parent, child1;
CREATE TABLE parent (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part)
ON UPDATE RESTRICT ON DELETE RESTRICT,
PRIMARY KEY (id, no_part)
)
PARTITION BY LIST (no_part);
CREATE TABLE child1 (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
PRIMARY KEY (id, no_part),
CONSTRAINT child1 CHECK ((no_part = 1))
);
-- force an indexscan as get_relation_idx_constraint_oid() use the unique
-- index on (conrelid, contypid, conname) to scan pg_cosntraint
set enable_seqscan TO off;
set enable_bitmapscan TO off;
SELECT conname
FROM pg_constraint
WHERE conrelid = 'parent'::regclass <=== parent
AND conindid = 'parent_pkey'::regclass; <=== PK index
DROP TABLE
CREATE TABLE
CREATE TABLE
SET
SET
conname
parent_id_abc_no_part_fkey < WOOPS!
parent_pkey
(2 rows)
In consequence, when attaching the partition, the PK of child1 is not marked as
partition of the parent's PK, which is wrong. WORST, the PK of child1 is
actually unexpectedly marked as a partition of the parent's **self-FK**:
postgres=# ALTER TABLE ONLY parent ATTACH PARTITION child1
FOR VALUES IN ('1');
SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;
ALTER TABLE
oid | conname | conparentid | conrelid | confrelid
---+-+-+--+---
16700 | parent_pkey | 0 |16695 | 0
16701 | parent_id_abc_no_part_fkey | 0 |16695 | 16695
16706 | child1 | 0 |16702 | 0
16708 | **child1_pkey** | **16701** |16702 | 0
16709 | parent_id_abc_no_part_fkey1 | 16701 |16695 | 16702
16712 | parent_id_abc_no_part_fkey | 16701 |16702 | 16695
(6 rows)
The expected result should probably be something like:
oid | conname | conparentid | conrelid | confrelid
---+-+-+--+---
16700 | parent_pkey | 0 |16695 | 0
...
16708 | child1_pkey | 16700 |16702 | 0
I suppose this bug might
Re: logical decoding and replication of sequences
On Tue, Aug 23, 2022 at 12:33 AM Thomas Munro wrote: > On Tue, Aug 23, 2022 at 4:21 PM Michael Paquier wrote: > > On Mon, Aug 08, 2022 at 06:15:46PM +1200, Thomas Munro wrote: > > > The attached patch that simply moves the cache invalidation into > > > report_invalid_record(), so that it's reached by the above code and > > > everything else that reports an error, seems to fix the problem in > > > src/bin/pg_ctl/t/003_promote.pl with Noah's spanner-in-the-works patch > > > applied, and passes check-world without it. I need to look at this > > > some more, though, and figure out if it's the right fix. > > > > Thomas, where are you on this open item? A potential PANIC at > > promotion is bad. One possible exit path would be to switch the > > default of recovery_prefetch, though that's a kind of last-resort > > option seen from here. > > I will get a fix committed this week -- I need to study > Horiguchi-san's analysis... Hi! I haven't been paying attention to this thread, but my attention was just drawn to it, and I'm wondering if the issue you're trying to track down here is actually the same as what I reported yesterday here: https://www.postgresql.org/message-id/CA+TgmoY0Lri=fCueg7m_2R_bSspUb1F8OFycEGaHNJw_EUW-=q...@mail.gmail.com -- Robert Haas EDB: http://www.enterprisedb.com
Re: SQL/JSON features for v15
On Mon, Aug 22, 2022 at 9:52 PM Jonathan S. Katz wrote: > > Andres, Robert: Do these changes address your concerns about the use of > > substransactions and reduce the risk of xid wraparound? > > Andres, Andrew, Amit, Robert -- as you have either worked on this or > expressed opinions -- any thoughts on this current patch set? I do not think that using subtransactions as part of the expression evaluation process is a sound idea pretty much under any circumstances. Maybe if the subtransations aren't commonly created and don't usually get XIDs there wouldn't be a big problem in practice, but it's an awfully heavyweight operation to be done inside expression evaluation even in corner cases. I think that if we need to make certain operations that would throw errors not throw errors, we need to refactor interfaces until it's possible to return an error indicator up to the appropriate level, not just let the error be thrown and catch it. The patches in question are thousands of lines of new code that I simply do not have time or interest to review in detail. I didn't commit this feature, or write this feature, or review this feature. I'm not familiar with any of the code. To really know what's going on here, I would need to review not only the new patches but also all the code in the original commits, and probably some of the preexisting code from before those commits that I have never examined in the past. That would take me quite a few months even if I had absolutely nothing else to do. And because I haven't been involved in this patch set in any way, I don't think it's really my responsibility. At the end of the day, the RMT is going to have to take a call here. It seems to me that Andres's concerns about code quality and lack of comments are probably somewhat legitimate, and in particular I do not think the use of subtransactions is a good idea. I also don't think that trying to fix those problems or generally improve the code by committing thousands of lines of new code in August when we're targeting a release in September or October is necessarily a good idea. But I'm also not in a position to say that the project is going to be irreparably damaged if we just ship what we've got, perhaps after fixing the most acute problems that we currently know about. This is after all relatively isolated from the rest of the system. Fixing the stuff that touches the core executor is probably pretty important, but beyond that, the worst thing that happens is the feature sucks and people who try to use it have bad experiences. That would be bad, and might be a sufficient reason to revert, but it's not nearly as bad as, say, the whole system being slow, or data loss for every user, or something like that. And we do have other bad code in the system. Is this a lot worse? I'm not in a position to say one way or the other. -- Robert Haas EDB: http://www.enterprisedb.com
Re: SQL/JSON features for v15
On 8/23/22 12:13 AM, Michael Paquier wrote: On Mon, Aug 22, 2022 at 07:57:29PM -0700, Andres Freund wrote: To me it feels like there's a probably too much work here to cram it at this point. If several other committers shared the load of working on this it'd perhaps be doable, but I've not seen many volunteers. While 0002 is dead simple, I am worried about the complexity created by 0001, 0003 (particularly tightening subtransactions with a CASE_EEOP) and 0004 at this late stage of the release process: https://www.postgresql.org/message-id/7d83684b-7932-9f29-400b-0beedfafc...@postgrespro.ru This is not a good sign after three betas for a feature as complex as this one. I see Amit is taking a closer look at the patches. The RMT had its regular meeting today and discussed the state of progress on this and how it reflects release timing. Our feeling is that regardless if the patchset is included/reverted, it would necessitate a Beta 4 (to be discussed with release team). While no Beta 4 date is set, given where we are this would probably push the release into early October to allow for adequate testing time. To say it another way, if we want to ensure we can have a 15.1 in the November update releases, we need to make a decision soon on how we want to proceed. Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: making relfilenodes 56 bits
On Tue, Aug 23, 2022 at 2:06 AM Dilip Kumar wrote: > OTOH, if we keep the two separate ranges for the user and system table > then we don't need all this complex logic of conflict checking. True. That's the downside. The question is whether it's worth adding some complexity to avoid needing separate ranges. Honestly, if we don't care about having separate ranges, we can do something even simpler and just make the starting relfilenumber for system tables same as the OID. Then we don't have to do anything at all, outside of not changing the OID assigned to pg_largeobject in a future release. Then as long as pg_upgrade is targeting a new cluster with completely fresh databases that have not had any system table rewrites so far, there can't be any conflict. And perhaps that is the best solution after all, but while it is simple in terms of code, I feel it's a bit complicated for human beings. It's very simple to understand the scheme that Amit proposed: if there's anything in the new cluster that would conflict, we move it out of the way. We don't have to assume the new cluster hasn't had any table rewrites. We don't have to nail down starting relfilenumber assignments for system tables. We don't have to worry about relfilenumber or OID assignments changing between releases. pg_largeobject is not a special case. There are no special ranges of OIDs or relfilenumbers required. It just straight up works -- all the time, no matter what, end of story. The other schemes we're talking about here all require a bunch of assumptions about stuff like what I just mentioned. We can certainly do it that way, and maybe it's even for the best. But I feel like it's a little bit fragile. Maybe some future change gets blocked because it would break one of the assumptions that the system relies on, or maybe someone doesn't even realize there's an issue and changes something that introduces a bug into this system. Or on the other hand maybe not. But I think there's at least some value in considering whether adding a little more code might actually make things simpler to reason about, and whether that might be a good enough reason to do it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: archive modules
Would this addition to the documentation be ok ? I hope the english is not too broken .. From 8ea8c21413eeac8fbd37527e64820cbdca3a5d7a Mon Sep 17 00:00:00 2001 From: benoit Date: Mon, 22 Aug 2022 12:00:46 +0200 Subject: [PATCH] basic_archive parameter visibility doc patch Module parameters are only visible from the pg_settings view once the module is loaded. Since an archive module is loaded by the archiver process, the parameters are never visible from the view. This patch adds a note bout this in the basic_archive module and system views documentation. --- doc/src/sgml/basic-archive.sgml | 13 + doc/src/sgml/system-views.sgml | 5 - 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/basic-archive.sgml b/doc/src/sgml/basic-archive.sgml index 0b650f17a8..65e70b795b 100644 --- a/doc/src/sgml/basic-archive.sgml +++ b/doc/src/sgml/basic-archive.sgml @@ -68,6 +68,19 @@ basic_archive.archive_directory = '/path/to/archive/directory' to any archiving still in progress, but users should use extra caution when doing so. + + + The archive module is loaded by the archiver process. Therefore, the + parameters defined in the module are not set outside this process and cannot + be seen from the pg_settings view or the + \dconfig meta-command. + These parameters values can be shown from the server's configuration + file(s) through the pg_file_settings view. + If you want to check the actual values applied by the archiver, you can + LOAD the module before reading + pg_settings. It's also possible to search + for the options directly with the SHOW command. + diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml index 9728039e71..c8f0f3843c 100644 --- a/doc/src/sgml/system-views.sgml +++ b/doc/src/sgml/system-views.sgml @@ -3275,7 +3275,10 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx This view does not display customized options - until the extension module that defines them has been loaded. + until the extension module that defines them has been loaded. For instance, + since an archive module is loaded by the archiver process, its customized + options are not visible from other sessions, unless they load the module + themselves. -- 2.37.1
Re: Letter case of "admin option"
On Mon, Aug 22, 2022 at 9:29 PM Kyotaro Horiguchi
wrote:
> Today, I see some error messages have been added, two of which look
> somewhat inconsistent.
>
> commands/user.c
> @707:
> >errmsg("must have admin option on role \"%s\" to add members",
> @1971:
> >errmsg("grantor must have ADMIN OPTION on \"%s\"",
>
> A grep'ing told me that the latter above is the only outlier among 6
> occurrences in total of "admin option/ADMIN OPTION".
>
> Don't we unify them? I slightly prefer "ADMIN OPTION" but no problem
> with them being in small letters. (Attached).
Fair point. There's some ambiguity in my mind about exactly how we
want to refer to this, which is probably why the messages ended up not
being entirely consistent. I feel like it's a little weird that we
talk about ADMIN OPTION as if it were a thing that you can possess.
For example, consider EXPLAIN. If you were trying to troubleshoot a
problem with a query plan, you wouldn't tell them "hey, please run
EXPLAIN, and be sure to use the ANALYZE OPTION". You would tell them
"hey, please run EXPLAIN, and be sure to use the ANALYZE option". In
that case, it's clear that the thing you need to include in the
command is ANALYZE -- which is an option -- not a thing called ANALYZE
OPTION.
In the case of GRANT, that's more ambiguous, because the word OPTION
actually appears in the syntax. But isn't that sort of accidental?
It's quite possible to give someone the right to administer a role
without ever mentioning the OPTION keyword:
rhaas=# create role bob;
CREATE ROLE
rhaas=# create role accounting admin bob;
CREATE ROLE
rhaas=# select roleid::regrole, member::regrole, grantor::regrole,
admin_option from pg_auth_members where roleid =
'accounting'::regrole;
roleid | member | grantor | admin_option
++-+--
accounting | bob| rhaas | t
(1 row)
You can't change this after-the-fact with ALTER ROLE or ALTER GROUP,
but if we added that ability, I imagine that the syntax would probably
not involve the OPTION keyword. You'd probably say something like:
ALTER ROLE accounting ADD ADMIN fred, or ALTER GROUP accounting DROP
ADMIN bob.
In short, I'm wondering whether we should regard ADMIN as the name of
the option, but OPTION as part of the GRANT syntax, and hence
capitalize it "ADMIN option". However, if the non-English speakers on
this list have a strong preference for something else I'm certainly
not going to fight about it.
> In passing, I met the following code in the same file.
>
> > if (!have_createrole_privilege() &&
> > !is_admin_of_role(currentUserId, roleid))
> > ereport(ERROR,
> >
> > (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> >errmsg("must have admin option on
> > role \"%s\"",
> > rolename)));
>
> The message seems a bit short that it only mentions admin option while
> omitting CREATEROLE privilege. "must have CREATEROLE privilege or
> admin option on role %s" might be better. Or we could say just
> "insufficient privilege" or "permission denied" in the main error
> message then provide "CREATEROLE privilege or admin option on role %s
> is required" in DETAILS or HINTS message. The message was added by
> c33d575899 along with the have_createrole_privilege() call so it is
> unclear to me whether it is intentional or not.
Yeah, I wasn't sure what to do about this. We do not mention superuser
privileges in every message where they theoretically apply, because it
would make a lot of messages longer for not much benefit. CREATEROLE
is a similar case and I think, but am not sure, that we treat it
similarly. So in my mind it is a judgement call what to do here, and
if other people think that what I picked wasn't best, we can change
it.
For what it's worth, I'm hoping to eventually remove the CREATEROLE
exception here. The superuser exception will remain, of course.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: standby promotion can create unreadable WAL
On Mon, Aug 22, 2022 at 10:38 PM Nathan Bossart wrote: > There was some previous discussion on this [0] [1]. > > [0] https://postgr.es/m/2B4510B2-3D70-4990-BFE3-0FE64041C08A%40amazon.com > [1] > https://postgr.es/m/20220127.100738.1985658263632578184.horikyota.ntt%40gmail.com Thanks. It seems like there are various doubts on those threads about whether EndRecPtr is really the right thing, but I'm not able to understand what the problem is exactly. Certainly, in the common case, it is, and as far as I can tell from looking at the code, it's what we're intended to use. So I would like to see some concrete evidence of it being wrong before we conclude that we need to do anything other than a trivial change. But the main issue for this thread is that we seem to be generating invalid WAL. That seems like something we'd better get fixed. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Schema variables - new implementation for Postgres 15
On Tue, Aug 23, 2022 at 11:27:45AM +0200, Pavel Stehule wrote:
> út 23. 8. 2022 v 7:56 odesílatel Julien Rouhaud napsal:
>
> >
> > I've been thinking a bit more about the shadowing, and one scenario we
> > didn't
> > discuss is something like this naive example:
> >
> > CREATE TABLE tt(a text, b text);
> >
> > CREATE TYPE abc AS (a text, b text, c text);
> > CREATE VARIABLE tt AS abc;
> >
> > INSERT INTO tt SELECT 'a', 'b';
> > LET tt = ('x', 'y', 'z');
> >
> > SELECT tt.a, tt.b, tt.c FROM tt;
> >
> > Which, with the default configuration, currently returns
> >
> > a | b | c
> > ---+---+---
> > a | b | z
> > (1 row)
> >
> > I feel a bit uncomfortable that the system allows mixing variable
> > attributes
> > and relation columns for the same relation name. This is even worse here
> > as
> > part of the variable attributes are shadowed.
> >
> > It feels like a good way to write valid queries that clearly won't do what
> > you
> > think they do, a bit like the correlated sub-query trap, so maybe we should
> > have a way to prevent it.
> >
> > What do you think?
> >
>
> I thought about it before. I think valid RTE (but with the missing column)
> can shadow the variable too.
>
> With this change your query fails:
>
> (2022-08-23 11:05:55) postgres=# SELECT tt.a, tt.b, tt.c FROM tt;
> ERROR: column tt.c does not exist
> LINE 1: SELECT tt.a, tt.b, tt.c FROM tt;
>^
> (2022-08-23 11:06:03) postgres=# set session_variables_ambiguity_warning to
> on;
> SET
> (2022-08-23 11:06:19) postgres=# SELECT tt.a, tt.b, tt.c FROM tt;
> WARNING: session variable "tt.a" is shadowed
> LINE 1: SELECT tt.a, tt.b, tt.c FROM tt;
>^
> DETAIL: Session variables can be shadowed by columns, routine's variables
> and routine's arguments with the same name.
> WARNING: session variable "tt.b" is shadowed
> LINE 1: SELECT tt.a, tt.b, tt.c FROM tt;
> ^
> DETAIL: Session variables can be shadowed by columns, routine's variables
> and routine's arguments with the same name.
> WARNING: session variable "public.tt" is shadowed
> LINE 1: SELECT tt.a, tt.b, tt.c FROM tt;
>^
> DETAIL: Session variables can be shadowed by tables or table's aliases
> with the same name.
> ERROR: column tt.c does not exist
> LINE 1: SELECT tt.a, tt.b, tt.c FROM tt;
Great, thanks a lot!
Could you add some regression tests for that scenario in the next version,
since this is handled by some new code? It will also probably be useful to
remind any possible committer about that choice.
Re: Letter case of "admin option"
On 2022-Aug-23, Kyotaro Horiguchi wrote:
> commands/user.c
> @707:
> >errmsg("must have admin option on role \"%s\" to add members",
> @1971:
> >errmsg("grantor must have ADMIN OPTION on \"%s\"",
>
> A grep'ing told me that the latter above is the only outlier among 6
> occurrences in total of "admin option/ADMIN OPTION".
>
> Don't we unify them? I slightly prefer "ADMIN OPTION" but no problem
> with them being in small letters. (Attached).
As a translator, it makes a huge difference to have them in upper vs.
lower case. In the former case I would keep it untranslated, while in
the latter I would translate it. Given that these are keywords to use
in a command, I think making them uppercase is the better approach.
I see several other messages using "admin option" in lower case in
user.c. The Spanish translation contains one translation already and it
is somewhat disappointing; I would prefer to have it as uppercase there
too.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Tracking last scan time
On Tue, 23 Aug 2022 at 11:00, Dave Page wrote: > > Often it is beneficial to review one's schema with a view to removing indexes > (and sometimes tables) that are no longer required. It's very difficult to > understand when that is the case by looking at the number of scans of a > relation as, for example, an index may be used infrequently but may be > critical in those times when it is used. I think this is easy to answer in a prometheus/datadog/etc world since you can consult the history of the count to see when it was last incremented. (Or do effectively that continously). I guess that just reinforces the idea that it should be optional. Perhaps there's room for some sort of general feature for controlling various time series aggregates like max() and min() sum() or, uhm, timeoflastchange() on whatever stats you want. That would let us remove a bunch of stuff from pg_stat_statements and let users turn on just the ones they want. And also let users enable things like time of last rollback or conflict etc. But that's just something to think about down the road. > The attached patch against HEAD adds optional tracking of the last scan time > for relations. It updates pg_stat_*_tables with new last_seq_scan and > last_idx_scan columns, and pg_stat_*_indexes with a last_idx_scan column to > help with this. > > Due to the use of gettimeofday(), those values are only maintained if a new > GUC, track_scans, is set to on. By default, it is off. Bikeshedding warning -- "track_scans" could equally apply to almost any stats about scans. I think the really relevant thing here is the times, not the scans. I think the GUC should be "track_scan_times". Or could that still be confused with scan durations? Maybe "track_scan_timestamps"? You could maybe make the gettimeofday cheaper by doing it less often. Like, skipping the increment if the old timestamp is newer than 1s before the transaction start time (I think that's available free if some other guc is enabled but I don't recall). Or isn't this cb normally happening after transaction end? So xactStopTimestamp might be available already? -- greg
Re: identifying the backend that owns a temporary schema
On Tue, 23 Aug 2022 at 05:29, Greg Stark wrote: > Having this function would be great (I admit I never responded because > I never figured out if your suggestion was right or not:). But should > it also be added to the pg_stat_activity view? Perhaps even just in > the SQL view using the function. > > Alternately should pg_stat_activity show the actual temp schema name > instead of the id? I don't recall if it's visible outside the backend > but if it is, could pg_stat_activity show whether the temp schema is > actually attached or not? > Would it work to cast the schema oid to type regnamespace? Then the actual data (numeric oid) would be present in the view, but it would display as text.
Re: making relfilenodes 56 bits
On Tue, Aug 23, 2022 at 3:16 PM Amit Kapila wrote: > > > OTOH, if we keep the two separate ranges for the user and system table > > then we don't need all this complex logic of conflict checking. From > > the old cluster, we can just remember the relfilenumbr of the > > pg_largeobject, and in the new cluster before trying to restore we can > > just query the new cluster pg_class and find out whether it is used by > > any system table and if so then we can just rewrite that system table. > > > > Before re-write of that system table, I think we need to set > NextRelFileNumber to a number greater than the max relfilenumber from > the old cluster, otherwise, it can later conflict with some user > table. Yes we will need to do that. > > And I think using two ranges might not be that complicated because as > > soon as we are done with the initdb we can just set NextRelFileNumber > > to the first user range relfilenumber so I think this could be the > > simplest solution. And I think what Amit is suggesting is something > > on this line? > > > > Yeah, I had thought of checking only pg_largeobject. I think the > advantage of having separate ranges is that we have a somewhat simpler > logic in the upgrade but OTOH the other scheme has the advantage of > having a single allocation scheme. Do we see any other pros/cons of > one over the other? I feel having a separate range is not much different from having a single allocation scheme, after cluster initialization, we will just have to set the NextRelFileNumber to something called FirstNormalRelFileNumber which looks fine to me. > One more thing we may want to think about is what if there are tables > created by extension? For example, I think BDR creates some tables > like node_group, conflict_history, etc. Now, I think if such an > extension is created by default, both old and new clusters will have > those tables. Isn't there a chance of relfilenumber conflict in such > cases? Shouldn't they behave as a normal user table? because before upgrade anyway new cluster can not have any table other than system tables and those tables created by an extension should also be restored as other user table does. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Tracking last scan time
Often it is beneficial to review one's schema with a view to removing indexes (and sometimes tables) that are no longer required. It's very difficult to understand when that is the case by looking at the number of scans of a relation as, for example, an index may be used infrequently but may be critical in those times when it is used. The attached patch against HEAD adds optional tracking of the last scan time for relations. It updates pg_stat_*_tables with new last_seq_scan and last_idx_scan columns, and pg_stat_*_indexes with a last_idx_scan column to help with this. Due to the use of gettimeofday(), those values are only maintained if a new GUC, track_scans, is set to on. By default, it is off. I did run a 12 hour test to see what the performance impact is. pgbench was run with scale factor 1 and 75 users across 4 identical bare metal machines running Rocky 8 in parallel which showed roughly a -2% average performance penalty against HEAD with track_scans enabled. Machines were PowerEdge R7525's with 128GB RAM, dual 16C/32T AMD 7302 CPUs, with the data directory on 6 x 800GB 12Gb/s SSD SAS drives in RAID 0. Kernel time source is tsc. HEAD track_scans Penalty (%) box1 19582.4973519341.8881 -1.22869541 box2 19936.5551319928.07479-0.04253664659 box3 19631.7889518649.64379-5.002830696 box4 19810.8676719420.67192-1.969604525 Average 19740.4272819335.06965-2.05343896 Doc and test updates included. -- Dave Page Blog: https://pgsnake.blogspot.com Twitter: @pgsnake EDB: https://www.enterprisedb.com last_scan_v1.diff Description: Binary data
