Re: remaining sql/json patches
Hi Erik, On Thu, Nov 16, 2023 at 13:52 Erik Rijkers wrote: > Op 11/15/23 om 14:00 schreef Amit Langote: > > Hi, > > [..] > > > Attached updated patch. The version of 0001 that I posted on Oct 11 > > to add the error-safe version of CoerceViaIO contained many > > unnecessary bits that are now removed. > > > > -- > > Thanks, Amit Langote > > EDB: http://www.enterprisedb.com > > > [v24-0001-Add-soft-error-handling-to-some-expression-nodes.patch] > > [v24-0002-Add-soft-error-handling-to-populate_record_field.patch] > > [v24-0003-SQL-JSON-query-functions.patch] > > [v24-0004-JSON_TABLE.patch] > > [v24-0005-Claim-SQL-standard-compliance-for-SQL-JSON-featu.patch] > > Hi Amit, > > Here is a statement that seems to gobble up all memory and to totally > lock up the machine. No ctrl-C - only a power reset gets me out of that. > It was in one of my tests, so it used to work: > > select json_query( > jsonb '"[3,4]"' >, '$[*]' returning bigint[] empty object on error > ); > > Can you have a look? Wow, will look. Thanks. >
Re: remaining sql/json patches
Hi Erik, On Sat, Nov 11, 2023 at 11:52 Erik Rijkers wrote: > Hi, > > At the moment, what is the patchset to be tested? The latest SQL/JSON > server I have is from September, and it's become unclear to me what > belongs to the SQL/JSON patchset. It seems to me cfbot erroneously > shows green because it successfully compiles later detail-patches (i.e., > not the SQL/JSON set itself). Please correct me if I'm wrong and it is > in fact possible to derive from cfbot a patchset that are the ones to > use to build the latest SQL/JSON server. I’ll be posting a new set that addresses Andres’ comments early next week. >
Re: remaining sql/json patches
Hi, On Thu, Oct 26, 2023 at 9:20 PM Nikita Malakhov wrote: > > Hi, > > The main goal was to correctly process invalid queries (as in examples above). > I'm not sure this could be done in type input functions. I thought that some > coercions could be checked before evaluating expressions for saving reasons. I assume by "invalid" you mean queries specifying types in RETURNING that don't support soft-error handling in their input function. Adding a check makes sense but its implementation should include a type cache interface to check whether a given type has error-safe input handling, possibly as a separate patch. IOW, the SQL/JSON patch shouldn't really make a list of types to report as unsupported. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
Hi Nikita, On Thu, Oct 26, 2023 at 2:13 AM Nikita Malakhov wrote: > Amit, on previous email, patch #2 - I agree that it is not the best idea to > introduce > new type of logic into the parser, so this logic could be moved to the > executor, > or removed at all. What do you think of these options? Yes maybe, though I'd first like to have a good answer to why is that logic necessary at all. Maybe you think it's better to emit an error in the SQL/JSON layer of code than in the type input function if it's unsafe? -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: run pgindent on a regular basis / scripted manner
On Tue, Oct 24, 2023 at 9:51 Jeff Davis wrote: > On Wed, 2023-10-18 at 22:34 +1300, David Rowley wrote: > > It would be good to learn how many of the committers out of the ones > > you listed that --enable-indent-checks would have saved from breaking > > koel. > > I'd find that a useful option. +1. While I’ve made it part of routine to keep my local work pgindented since breaking Joel once, an option like this would still be useful. >
Re: remaining sql/json patches
On Mon, Oct 16, 2023 at 5:21 PM Nikita Malakhov wrote:
>
> Hi!
>
> With the latest set of patches we encountered failure with the following
> query:
>
> postgres@postgres=# SELECT JSON_QUERY(jsonpath '"aaa"', '$' RETURNING text);
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> The connection to the server was lost. Attempting reset: Failed.
> Time: 11.165 ms
>
> A colleague of mine, Anton Melnikov, proposed the following changes which
> slightly
> alter coercion functions to process this kind of error correctly.
>
> Please check attached patch set.
Thanks for the patches.
I think I understand patch 1. It makes each of JSON_{QUERY | VALUE |
EXISTS}() use FORMAT JSON for the context item by default, which I
think is the correct behavior.
As for patch 2, maybe the executor part is fine, but I'm not so sure
about the parser part. Could you please explain why you think the
parser must check error-safety of the target type for allowing IO
coercion for non-ERROR behaviors?
Even if we consider that that's what should be done, it doesn't seem
like a good idea for the parser to implement its own logic for
determining error-safety. IOW, the parser should really be using some
type cache API. I thought there might have been a flag in pg_proc
(prosafe) or pg_type (typinsafe), but apparently there isn't.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
Hi Anton,
On Tue, Oct 17, 2023 at 4:11 PM Anton A. Melnikov
wrote:
> On 17.10.2023 07:02, Amit Langote wrote:
>
> > One thing jian he missed during the debugging is that
> > ExecEvalJsonExprCoersion() receives the EMPTY ARRAY value via
> > *op->resvalue/resnull, set by ExecEvalJsonExprBehavior(), because
> > that's the ON EMPTY behavior specified in the constraint. The bug was
> > that the code in ExecEvalJsonExprCoercion() failed to set val_string
> > to that value ("[]") before passing to InputFunctionCallSafe(), so the
> > latter would assume the input is NULL.
> >
> Thank a lot for this remark!
>
> I tried to dig to the transformJsonOutput() to fix it earlier at the analyze
> stage,
> but it looks like a rather hard way.
Indeed. As I said, the problem was a bug in ExecEvalJsonExprCoercion().
>
> Maybe simple in accordance with you note remove the second condition from
> this line:
> if (jb && JB_ROOT_IS_SCALAR(jb)) ?
Yeah, that's how I would fix it.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Mon, Oct 16, 2023 at 10:44 PM Anton A. Melnikov
wrote:
> On 16.10.2023 15:49, jian he wrote:
> > add the following code after ExecEvalJsonExprCoercion if
> > (!InputFunctionCallSafe(...) works, but seems like a hack.
> >
> > if (!val_string)
> > {
> > *op->resnull = true;
> > *op->resvalue = (Datum) 0;
> > }
>
> It seems the constraint should work here:
>
> After
>
> CREATE TABLE test (
> js text,
> i int,
> x jsonb DEFAULT JSON_QUERY(jsonb '[1,2]', '$[*]' WITH WRAPPER)
> CONSTRAINT test_constraint
> CHECK (JSON_QUERY(js::jsonb, '$.a' RETURNING char(5) OMIT
> QUOTES EMPTY ARRAY ON EMPTY) > 'a')
> );
>
> INSERT INTO test_jsonb_constraints VALUES ('[]');
>
> one expected to see an error like that:
>
> ERROR: new row for relation "test" violates check constraint
> "test_constraint"
> DETAIL: Failing row contains ([], null, [1, 2]).
>
> not "INSERT 0 1"
Yes, the correct thing here is for the constraint to fail.
One thing jian he missed during the debugging is that
ExecEvalJsonExprCoersion() receives the EMPTY ARRAY value via
*op->resvalue/resnull, set by ExecEvalJsonExprBehavior(), because
that's the ON EMPTY behavior specified in the constraint. The bug was
that the code in ExecEvalJsonExprCoercion() failed to set val_string
to that value ("[]") before passing to InputFunctionCallSafe(), so the
latter would assume the input is NULL.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
Hi,
On Mon, Oct 16, 2023 at 5:34 PM Nikita Malakhov wrote:
>
> Hi,
>
> Also FYI - the following case results in segmentation fault:
>
> postgres@postgres=# CREATE TABLE test_jsonb_constraints (
> js text,
> i int,
> x jsonb DEFAULT JSON_QUERY(jsonb '[1,2]', '$[*]' WITH WRAPPER)
> CONSTRAINT test_jsonb_constraint1
> CHECK (js IS JSON)
> CONSTRAINT test_jsonb_constraint5
> CHECK (JSON_QUERY(js::jsonb, '$.mm' RETURNING char(5) OMIT
> QUOTES EMPTY ARRAY ON EMPTY) > 'a' COLLATE "C")
> CONSTRAINT test_jsonb_constraint6
> CHECK (JSON_EXISTS(js::jsonb, 'strict $.a' RETURNING int TRUE
> ON ERROR) < 2)
> );
> CREATE TABLE
> Time: 13.518 ms
> postgres@postgres=# INSERT INTO test_jsonb_constraints VALUES ('[]');
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> The connection to the server was lost. Attempting reset: Failed.
> Time: 6.858 ms
> @!>
>
> We're currently looking into this case.
Thanks for the report. I think I've figured out the problem --
ExecEvalJsonExprCoercion() mishandles the EMPTY ARRAY ON EMPTY case.
I'm reading the other 2 patches...
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Wed, Oct 11, 2023 at 2:08 PM Amit Langote wrote: > On Sat, Oct 7, 2023 at 6:49 AM Andres Freund wrote: > > On 2023-09-29 13:57:46 +0900, Amit Langote wrote: > > > Thanks. I will push the attached 0001 shortly. > > > > Sorry for not looking at this earlier. > > Thanks for the review. Replying here only to your comments on 0001. > > > Have you done benchmarking to verify that 0001 does not cause performance > > regressions? I'd not be suprised if it did. > > I found that it indeed did once I benchmarked with something that > would stress EEOP_IOCOERCE: > > do $$ > begin > for i in 1..2000 loop > i := i::text; > end loop; end; $$ language plpgsql; > DO > > Times and perf report: > > HEAD: > > Time: 1815.824 ms (00:01.816) > Time: 1818.980 ms (00:01.819) > Time: 1695.555 ms (00:01.696) > Time: 1762.022 ms (00:01.762) > > --97.49%--exec_stmts > | > --85.97%--exec_assign_expr > | > |--65.56%--exec_eval_expr > | | > | |--53.71%--ExecInterpExpr > | | | > | | |--14.14%--textin > > > Patched: > > Time: 1872.469 ms (00:01.872) > Time: 1927.371 ms (00:01.927) > Time: 1910.126 ms (00:01.910) > Time: 1948.322 ms (00:01.948) > > --97.70%--exec_stmts > | > --88.13%--exec_assign_expr > | > |--73.27%--exec_eval_expr > | | > | |--58.29%--ExecInterpExpr > | | | > | | > |--25.69%--InputFunctionCallSafe > | | | | > | | | > |--14.75%--textin > > So, yes, putting InputFunctionCallSafe() in the common path may not > have been such a good idea. > > > I'd split the soft-error path into > > a separate opcode. For JIT it can largely be implemented using the same > > code, > > eliding the check if it's the non-soft path. Or you can just put it into an > > out-of-line function. > > Do you mean putting the execExprInterp.c code for the soft-error path > (with a new opcode) into an out-of-line function? That definitely > makes the JIT version a tad simpler than if the error-checking is done > in-line. > > So, the existing code for EEOP_IOCOERCE in both execExprInterp.c and > llvmjit_expr.c will remain unchanged. Also, I can write the code for > the new opcode such that it doesn't use InputFunctionCallSafe() at > runtime, but rather passes the ErrorSaveContext directly by putting > that in the input function's FunctionCallInfo.context and checking > SOFT_ERROR_OCCURRED() directly. That will have less overhead. > > > I don't like adding more stuff to ExprState. This one seems particularly > > awkward, because it might be used by more than one level in an expression > > subtree, which means you really need to save/restore old values when > > recursing. > > Hmm, I'd think that all levels will follow either soft or non-soft > error mode, so sharing the ErrorSaveContext passed via ExprState > doesn't look wrong to me. IOW, there's only one value, not one for > every level, so there doesn't appear to be any need to have the > save/restore convention as we have for innermost_domainval et al. > > I can see your point that adding another 8 bytes at the end of > ExprState might be undesirable. Note though that ExprState.escontext > is only accessed in the ExecInitExpr phase, but during evaluation. > > The alternative to not passing the ErrorSaveContext via ExprState is > to add a new parameter to ExecInitExprRec() and to functions that call > it. The footprint would be much larger though. Would you rather > prefer that? > > > > @@ -1579,25 +1582,13 @@ ExecInitExprRec(Expr *node, ExprState *state, > > > > > > /* lookup the result type's input function > > > */ > > > scratch.d.iocoerce.finfo_in = > > > palloc0(sizeof(FmgrInfo)); > > > - scratch.d.iocoerce.fcinfo_data_in = > > > palloc0(SizeForFunctionCallInfo(3)); > > &
Re: remaining sql/json patches
Hi Andres, On Sat, Oct 7, 2023 at 6:49 AM Andres Freund wrote: > Hi, > > On 2023-09-29 13:57:46 +0900, Amit Langote wrote: > > Thanks. I will push the attached 0001 shortly. > > Sorry for not looking at this earlier. Thanks for the review. Replying here only to your comments on 0001. > Have you done benchmarking to verify that 0001 does not cause performance > regressions? I'd not be suprised if it did. I found that it indeed did once I benchmarked with something that would stress EEOP_IOCOERCE: do $$ begin for i in 1..2000 loop i := i::text; end loop; end; $$ language plpgsql; DO Times and perf report: HEAD: Time: 1815.824 ms (00:01.816) Time: 1818.980 ms (00:01.819) Time: 1695.555 ms (00:01.696) Time: 1762.022 ms (00:01.762) --97.49%--exec_stmts | --85.97%--exec_assign_expr | |--65.56%--exec_eval_expr | | | |--53.71%--ExecInterpExpr | | | | | |--14.14%--textin Patched: Time: 1872.469 ms (00:01.872) Time: 1927.371 ms (00:01.927) Time: 1910.126 ms (00:01.910) Time: 1948.322 ms (00:01.948) --97.70%--exec_stmts | --88.13%--exec_assign_expr | |--73.27%--exec_eval_expr | | | |--58.29%--ExecInterpExpr | | | | | |--25.69%--InputFunctionCallSafe | | | | | | | |--14.75%--textin So, yes, putting InputFunctionCallSafe() in the common path may not have been such a good idea. > I'd split the soft-error path into > a separate opcode. For JIT it can largely be implemented using the same code, > eliding the check if it's the non-soft path. Or you can just put it into an > out-of-line function. Do you mean putting the execExprInterp.c code for the soft-error path (with a new opcode) into an out-of-line function? That definitely makes the JIT version a tad simpler than if the error-checking is done in-line. So, the existing code for EEOP_IOCOERCE in both execExprInterp.c and llvmjit_expr.c will remain unchanged. Also, I can write the code for the new opcode such that it doesn't use InputFunctionCallSafe() at runtime, but rather passes the ErrorSaveContext directly by putting that in the input function's FunctionCallInfo.context and checking SOFT_ERROR_OCCURRED() directly. That will have less overhead. > I don't like adding more stuff to ExprState. This one seems particularly > awkward, because it might be used by more than one level in an expression > subtree, which means you really need to save/restore old values when > recursing. Hmm, I'd think that all levels will follow either soft or non-soft error mode, so sharing the ErrorSaveContext passed via ExprState doesn't look wrong to me. IOW, there's only one value, not one for every level, so there doesn't appear to be any need to have the save/restore convention as we have for innermost_domainval et al. I can see your point that adding another 8 bytes at the end of ExprState might be undesirable. Note though that ExprState.escontext is only accessed in the ExecInitExpr phase, but during evaluation. The alternative to not passing the ErrorSaveContext via ExprState is to add a new parameter to ExecInitExprRec() and to functions that call it. The footprint would be much larger though. Would you rather prefer that? > > @@ -1579,25 +1582,13 @@ ExecInitExprRec(Expr *node, ExprState *state, > > > > /* lookup the result type's input function */ > > scratch.d.iocoerce.finfo_in = > > palloc0(sizeof(FmgrInfo)); > > - scratch.d.iocoerce.fcinfo_data_in = > > palloc0(SizeForFunctionCallInfo(3)); > > - > > getTypeInputInfo(iocoerce->resulttype, > > - , > > ); > > + , > > ); > > fmgr_info(iofunc, > > scratch.d.iocoerce.finfo_in); > > fmgr_info_set_expr((Node *) node, > > scratch.d.iocoerce.finfo_in); > > - > > InitFunc
Re: remaining sql/json patches
On Fri, Oct 6, 2023 at 19:01 Alvaro Herrera wrote: > On 2023-Oct-06, Amit Langote wrote: > > > 2. Assignment of op->d.iocoerce.escontext needed to be changed like this: > > > > v_params[4] = > l_ptr_const(op->d.iocoerce.escontext, > > - > > l_ptr(StructErrorSaveContext)); > > + l_ptr(StructNode)); > > Oh, so you had to go back to using StructNode in order to get this > fixed? That's weird. Is it just because InputFunctionCallSafe is > defined to take fmNodePtr? (I still fail to see that a pointer to > ErrorSaveContext would differ in any material way from a pointer to > Node). The difference matters to LLVM’s type system, which considers Node to be a type with 1 sub-type (struct member) and ErrorSaveContext with 4 sub-types. It doesn’t seem to understand that both share the first member. Another think I thought was weird is that it would only crash in LLVM5 > debug and not the other LLVM-enabled animals, but looking closer at the > buildfarm results, I think that may have been only because you reverted > too quickly, and phycodorus and petalura didn't actually run with > 7fbc75b26ed8 before you reverted it. Dragonet did make a run with it, > but it's marked as "LLVM optimized" instead of "LLVM debug". I suppose > that must be making a difference. AFAICS, only assert-enabled LLVM builds crash. >
Re: remaining sql/json patches
On Wed, Oct 4, 2023 at 10:26 PM Amit Langote wrote:
> On Tue, Oct 3, 2023 at 10:11 PM Amit Langote wrote:
> > On Mon, Oct 2, 2023 at 2:26 PM Amit Langote wrote:
> > > On Mon, Oct 2, 2023 at 1:24 PM Amit Langote
> > > wrote:
> > > > Pushed this 30 min ago (no email on -committers yet!) and am looking
> > > > at the following llvm crash reported by buildfarm animal pogona [1]:
> > > > This seems to me to be complaining about the following addition:
> > > >
> > > > + {
> > > > + Oid ioparam = op->d.iocoerce.typioparam;
> > > > + LLVMValueRef v_params[6];
> > > > + LLVMValueRef v_success;
> > > > +
> > > > + v_params[0] =
> > > > l_ptr_const(op->d.iocoerce.finfo_in,
> > > > +
> > > > l_ptr(StructFmgrInfo));
> > > > + v_params[1] = v_output;
> > > > + v_params[2] = l_oid_const(lc, ioparam);
> > > > + v_params[3] = l_int32_const(lc, -1);
> > > > + v_params[4] =
> > > > l_ptr_const(op->d.iocoerce.escontext,
> > > > +
> > > > l_ptr(StructErrorSaveContext));
> > > >
> > > > - LLVMBuildStore(b, v_retval, v_resvaluep);
> > > > + /*
> > > > +* InputFunctionCallSafe() will write directly
> > > > into
> > > > +* *op->resvalue.
> > > > +*/
> > > > + v_params[5] = v_resvaluep;
> > > > +
> > > > + v_success = LLVMBuildCall(b, llvm_pg_func(mod,
> > > > "InputFunctionCallSafe"),
> > > > + v_params,
> > > > lengthof(v_params),
> > > > +
> > > > "funccall_iocoerce_in_safe");
> > > > +
> > > > + /*
> > > > +* Return null if InputFunctionCallSafe()
> > > > encountered
> > > > +* an error.
> > > > +*/
> > > > + v_resnullp = LLVMBuildICmp(b, LLVMIntEQ,
> > > > v_success,
> > > > + l_sbool_const(0),
> > > > "");
> > > > + }
> > >
> ...I haven't yet pinpointed down
> which of the LLVM's asserts it is, nor have I been able to walk
> through LLVM source code using gdb to figure what the new code is
> doing wrong. Maybe I'm still missing a trick or two...
I finally managed to analyze the crash by getting the correct LLVM build.
So the following bits are the culprits:
1. v_output needed to be converted from being reference to a Datum to
be reference to char * as follows before passing to
InputFunctionCallSafe():
- v_params[1] = v_output;
+ v_params[1] = LLVMBuildIntToPtr(b, v_output,
+
l_ptr(LLVMInt8TypeInContext(lc)),
+ "");
2. Assignment of op->d.iocoerce.escontext needed to be changed like this:
v_params[4] = l_ptr_const(op->d.iocoerce.escontext,
-
l_ptr(StructErrorSaveContext));
+ l_ptr(StructNode));
3. v_success needed to be "zero-extended" to match in type with
whatever s_bool_const() produces, as follows:
+ v_success = LLVMBuildZExt(b, v_success,
TypeStorageBool, "");
v_resnullp = LLVMBuildICmp(b, LLVMIntEQ, v_success,
l_sbool_const(0), "");
No more crashes with the above fixes.
Attached shows the delta against the patch I reverted. I'll push the
fixed up version on Monday.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
iocoerce-llvm-fixes.diff
Description: Binary data
Re: remaining sql/json patches
On Tue, Oct 3, 2023 at 10:11 PM Amit Langote wrote:
> On Mon, Oct 2, 2023 at 2:26 PM Amit Langote wrote:
> > On Mon, Oct 2, 2023 at 1:24 PM Amit Langote wrote:
> > > Pushed this 30 min ago (no email on -committers yet!) and am looking
> > > at the following llvm crash reported by buildfarm animal pogona [1]:
> > >
> > > #4 0x7f5bceb673d5 in __assert_fail_base (fmt=0x7f5bcecdbdc8
> > > "%s%s%s:%u: %s%sAssertion `%s' failed.\\n%n",
> > > assertion=assertion@entry=0x7f5bc1336419 "(i >= FTy->getNumParams() ||
> > > FTy->getParamType(i) == Args[i]->getType()) && \\"Calling a function
> > > with a bad signature!\\"", file=file@entry=0x7f5bc1336051
> > > "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp",
> > > line=line@entry=299, function=function@entry=0x7f5bc13362af "void
> > > llvm::CallInst::init(llvm::FunctionType *, llvm::Value *,
> > > ArrayRef, ArrayRef, const
> > > llvm::Twine &)") at ./assert/assert.c:92
> > > #5 0x7f5bceb763a2 in __assert_fail (assertion=0x7f5bc1336419 "(i
> > > >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType())
> > > && \\"Calling a function with a bad signature!\\"",
> > > file=0x7f5bc1336051
> > > "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp", line=299,
> > > function=0x7f5bc13362af "void llvm::CallInst::init(llvm::FunctionType
> > > *, llvm::Value *, ArrayRef,
> > > ArrayRef, const llvm::Twine &)") at
> > > ./assert/assert.c:101
> > > #6 0x7f5bc110f138 in llvm::CallInst::init (this=0x557a91f3e508,
> > > FTy=0x557a91ed9ae0, Func=0x557a91f8be88, Args=..., Bundles=...,
> > > NameStr=...) at
> > > /home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp:297
> > > #7 0x7f5bc0fa579d in llvm::CallInst::CallInst
> > > (this=0x557a91f3e508, Ty=0x557a91ed9ae0, Func=0x557a91f8be88,
> > > Args=..., Bundles=..., NameStr=..., InsertBefore=0x0) at
> > > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1934
> > > #8 0x7f5bc0fa538c in llvm::CallInst::Create (Ty=0x557a91ed9ae0,
> > > Func=0x557a91f8be88, Args=..., Bundles=..., NameStr=...,
> > > InsertBefore=0x0) at
> > > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1444
> > > #9 0x7f5bc0fa51f9 in llvm::IRBuilder > > llvm::IRBuilderDefaultInserter>::CreateCall (this=0x557a91f9c6a0,
> > > FTy=0x557a91ed9ae0, Callee=0x557a91f8be88, Args=..., Name=...,
> > > FPMathTag=0x0) at
> > > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1669
> > > #10 0x7f5bc100edda in llvm::IRBuilder > > llvm::IRBuilderDefaultInserter>::CreateCall (this=0x557a91f9c6a0,
> > > Callee=0x557a91f8be88, Args=..., Name=..., FPMathTag=0x0) at
> > > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1663
> > > #11 0x7f5bc100714e in LLVMBuildCall (B=0x557a91f9c6a0,
> > > Fn=0x557a91f8be88, Args=0x7ffde6fa0b50, NumArgs=6, Name=0x7f5bc30b648c
> > > "funccall_iocoerce_in_safe") at
> > > /home/bf/src/llvm-project-5/llvm/lib/IR/Core.cpp:2964
> > > #12 0x7f5bc30af861 in llvm_compile_expr (state=0x557a91fbeac0) at
> > > /home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/jit/llvm/llvmjit_expr.c:1373
> > >
> > > This seems to me to be complaining about the following addition:
> > >
> > > + {
> > > + Oid ioparam = op->d.iocoerce.typioparam;
> > > + LLVMValueRef v_params[6];
> > > + LLVMValueRef v_success;
> > > +
> > > + v_params[0] = l_ptr_const(op->d.iocoerce.finfo_in,
> > > + l_ptr(StructFmgrInfo));
> > > + v_params[1] = v_output;
> > > + v_params[2] = l_oid_const(lc, ioparam);
> > > + v_params[3] = l_int32_const(lc, -1);
> > > + v_params[4] =
> > > l_ptr_const(op->d.iocoerce.escontext,
> > > +
> > > l_ptr(StructErrorSaveContext));
> > >
> > > - LLVMBuildStore(b, v_retval, v_resvaluep);
> > > + /*
> > > +* InputFunctionCallSafe() will write directly
> > > into
> > > +* *op->resvalue.
> > > +
Re: remaining sql/json patches
On Mon, Oct 2, 2023 at 2:26 PM Amit Langote wrote:
> On Mon, Oct 2, 2023 at 1:24 PM Amit Langote wrote:
> > Pushed this 30 min ago (no email on -committers yet!) and am looking
> > at the following llvm crash reported by buildfarm animal pogona [1]:
> >
> > #4 0x7f5bceb673d5 in __assert_fail_base (fmt=0x7f5bcecdbdc8
> > "%s%s%s:%u: %s%sAssertion `%s' failed.\\n%n",
> > assertion=assertion@entry=0x7f5bc1336419 "(i >= FTy->getNumParams() ||
> > FTy->getParamType(i) == Args[i]->getType()) && \\"Calling a function
> > with a bad signature!\\"", file=file@entry=0x7f5bc1336051
> > "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp",
> > line=line@entry=299, function=function@entry=0x7f5bc13362af "void
> > llvm::CallInst::init(llvm::FunctionType *, llvm::Value *,
> > ArrayRef, ArrayRef, const
> > llvm::Twine &)") at ./assert/assert.c:92
> > #5 0x7f5bceb763a2 in __assert_fail (assertion=0x7f5bc1336419 "(i
> > >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType())
> > && \\"Calling a function with a bad signature!\\"",
> > file=0x7f5bc1336051
> > "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp", line=299,
> > function=0x7f5bc13362af "void llvm::CallInst::init(llvm::FunctionType
> > *, llvm::Value *, ArrayRef,
> > ArrayRef, const llvm::Twine &)") at
> > ./assert/assert.c:101
> > #6 0x7f5bc110f138 in llvm::CallInst::init (this=0x557a91f3e508,
> > FTy=0x557a91ed9ae0, Func=0x557a91f8be88, Args=..., Bundles=...,
> > NameStr=...) at
> > /home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp:297
> > #7 0x7f5bc0fa579d in llvm::CallInst::CallInst
> > (this=0x557a91f3e508, Ty=0x557a91ed9ae0, Func=0x557a91f8be88,
> > Args=..., Bundles=..., NameStr=..., InsertBefore=0x0) at
> > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1934
> > #8 0x7f5bc0fa538c in llvm::CallInst::Create (Ty=0x557a91ed9ae0,
> > Func=0x557a91f8be88, Args=..., Bundles=..., NameStr=...,
> > InsertBefore=0x0) at
> > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1444
> > #9 0x7f5bc0fa51f9 in llvm::IRBuilder > llvm::IRBuilderDefaultInserter>::CreateCall (this=0x557a91f9c6a0,
> > FTy=0x557a91ed9ae0, Callee=0x557a91f8be88, Args=..., Name=...,
> > FPMathTag=0x0) at
> > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1669
> > #10 0x7f5bc100edda in llvm::IRBuilder > llvm::IRBuilderDefaultInserter>::CreateCall (this=0x557a91f9c6a0,
> > Callee=0x557a91f8be88, Args=..., Name=..., FPMathTag=0x0) at
> > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1663
> > #11 0x7f5bc100714e in LLVMBuildCall (B=0x557a91f9c6a0,
> > Fn=0x557a91f8be88, Args=0x7ffde6fa0b50, NumArgs=6, Name=0x7f5bc30b648c
> > "funccall_iocoerce_in_safe") at
> > /home/bf/src/llvm-project-5/llvm/lib/IR/Core.cpp:2964
> > #12 0x7f5bc30af861 in llvm_compile_expr (state=0x557a91fbeac0) at
> > /home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/jit/llvm/llvmjit_expr.c:1373
> >
> > This seems to me to be complaining about the following addition:
> >
> > + {
> > + Oid ioparam = op->d.iocoerce.typioparam;
> > + LLVMValueRef v_params[6];
> > + LLVMValueRef v_success;
> > +
> > + v_params[0] = l_ptr_const(op->d.iocoerce.finfo_in,
> > + l_ptr(StructFmgrInfo));
> > + v_params[1] = v_output;
> > + v_params[2] = l_oid_const(lc, ioparam);
> > + v_params[3] = l_int32_const(lc, -1);
> > + v_params[4] = l_ptr_const(op->d.iocoerce.escontext,
> > +
> > l_ptr(StructErrorSaveContext));
> >
> > - LLVMBuildStore(b, v_retval, v_resvaluep);
> > + /*
> > +* InputFunctionCallSafe() will write directly into
> > +* *op->resvalue.
> > +*/
> > + v_params[5] = v_resvaluep;
> > +
> > + v_success = LLVMBuildCall(b, llvm_pg_func(mod,
> > "InputFunctionCallSafe"),
> > + v_params,
> > lengthof(v_params),
> > +
> > "funccall_iocoerce_in_safe");
> >
Re: remaining sql/json patches
On Mon, Oct 2, 2023 at 1:24 PM Amit Langote wrote:
> Pushed this 30 min ago (no email on -committers yet!) and am looking
> at the following llvm crash reported by buildfarm animal pogona [1]:
>
> #4 0x7f5bceb673d5 in __assert_fail_base (fmt=0x7f5bcecdbdc8
> "%s%s%s:%u: %s%sAssertion `%s' failed.\\n%n",
> assertion=assertion@entry=0x7f5bc1336419 "(i >= FTy->getNumParams() ||
> FTy->getParamType(i) == Args[i]->getType()) && \\"Calling a function
> with a bad signature!\\"", file=file@entry=0x7f5bc1336051
> "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp",
> line=line@entry=299, function=function@entry=0x7f5bc13362af "void
> llvm::CallInst::init(llvm::FunctionType *, llvm::Value *,
> ArrayRef, ArrayRef, const
> llvm::Twine &)") at ./assert/assert.c:92
> #5 0x7f5bceb763a2 in __assert_fail (assertion=0x7f5bc1336419 "(i
> >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType())
> && \\"Calling a function with a bad signature!\\"",
> file=0x7f5bc1336051
> "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp", line=299,
> function=0x7f5bc13362af "void llvm::CallInst::init(llvm::FunctionType
> *, llvm::Value *, ArrayRef,
> ArrayRef, const llvm::Twine &)") at
> ./assert/assert.c:101
> #6 0x7f5bc110f138 in llvm::CallInst::init (this=0x557a91f3e508,
> FTy=0x557a91ed9ae0, Func=0x557a91f8be88, Args=..., Bundles=...,
> NameStr=...) at
> /home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp:297
> #7 0x7f5bc0fa579d in llvm::CallInst::CallInst
> (this=0x557a91f3e508, Ty=0x557a91ed9ae0, Func=0x557a91f8be88,
> Args=..., Bundles=..., NameStr=..., InsertBefore=0x0) at
> /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1934
> #8 0x7f5bc0fa538c in llvm::CallInst::Create (Ty=0x557a91ed9ae0,
> Func=0x557a91f8be88, Args=..., Bundles=..., NameStr=...,
> InsertBefore=0x0) at
> /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1444
> #9 0x7f5bc0fa51f9 in llvm::IRBuilder llvm::IRBuilderDefaultInserter>::CreateCall (this=0x557a91f9c6a0,
> FTy=0x557a91ed9ae0, Callee=0x557a91f8be88, Args=..., Name=...,
> FPMathTag=0x0) at
> /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1669
> #10 0x7f5bc100edda in llvm::IRBuilder llvm::IRBuilderDefaultInserter>::CreateCall (this=0x557a91f9c6a0,
> Callee=0x557a91f8be88, Args=..., Name=..., FPMathTag=0x0) at
> /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1663
> #11 0x7f5bc100714e in LLVMBuildCall (B=0x557a91f9c6a0,
> Fn=0x557a91f8be88, Args=0x7ffde6fa0b50, NumArgs=6, Name=0x7f5bc30b648c
> "funccall_iocoerce_in_safe") at
> /home/bf/src/llvm-project-5/llvm/lib/IR/Core.cpp:2964
> #12 0x7f5bc30af861 in llvm_compile_expr (state=0x557a91fbeac0) at
>
/home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/jit/llvm/llvmjit_expr.c:1373
>
> This seems to me to be complaining about the following addition:
>
> + {
> + Oid ioparam = op->d.iocoerce.typioparam;
> + LLVMValueRef v_params[6];
> + LLVMValueRef v_success;
> +
> + v_params[0] = l_ptr_const(op->d.iocoerce.finfo_in,
> + l_ptr(StructFmgrInfo));
> + v_params[1] = v_output;
> + v_params[2] = l_oid_const(lc, ioparam);
> + v_params[3] = l_int32_const(lc, -1);
> + v_params[4] =
l_ptr_const(op->d.iocoerce.escontext,
> +
> l_ptr(StructErrorSaveContext));
>
> - LLVMBuildStore(b, v_retval, v_resvaluep);
> + /*
> +* InputFunctionCallSafe() will write directly
into
> +* *op->resvalue.
> +*/
> + v_params[5] = v_resvaluep;
> +
> + v_success = LLVMBuildCall(b, llvm_pg_func(mod,
> "InputFunctionCallSafe"),
> + v_params,
lengthof(v_params),
> +
"funccall_iocoerce_in_safe");
> +
> + /*
> +* Return null if InputFunctionCallSafe()
encountered
> +* an error.
> +*/
> + v_resnullp = LLVMBuildICmp(b, LLVMIntEQ,
v_success,
> + l_sbool_const(0), "");
> + }
Although most animals except pogona looked fine, I've decided to revert the
patch for now.
IIUC, LLVM is complaining that the co
Re: remaining sql/json patches
On Fri, Sep 29, 2023 at 1:57 PM Amit Langote wrote: > On Thu, Sep 28, 2023 at 8:04 PM Alvaro Herrera > wrote: > > On 2023-Sep-27, Amit Langote wrote: > > > Maybe the following is better: > > > > > > + /* > > > +* For expression nodes that support soft errors. Should be set to > > > NULL > > > +* before calling ExecInitExprRec() if the caller wants errors thrown. > > > +*/ > > > > > > ...as in the attached. > > > > That's good. > > > > > Alvaro, do you think your concern regarding escontext not being in the > > > right spot in the ExprState struct is addressed? It doesn't seem very > > > critical to me to place it in the struct's 1st cacheline, because > > > escontext is not accessed in performance critical paths such as during > > > expression evaluation, especially with the latest version. (It would > > > get accessed during evaluation with previous versions.) > > > > > > If so, I'd like to move ahead with committing it. > > > > Yeah, looks OK to me in v21. > > Thanks. I will push the attached 0001 shortly. Pushed this 30 min ago (no email on -committers yet!) and am looking at the following llvm crash reported by buildfarm animal pogona [1]: #0 __pthread_kill_implementation (threadid=, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44 44 ./nptl/pthread_kill.c: No such file or directory. #0 __pthread_kill_implementation (threadid=, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44 #1 0x7f5bcebcb15f in __pthread_kill_internal (signo=6, threadid=) at ./nptl/pthread_kill.c:78 #2 0x7f5bceb7d472 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #3 0x7f5bceb674b2 in __GI_abort () at ./stdlib/abort.c:79 #4 0x7f5bceb673d5 in __assert_fail_base (fmt=0x7f5bcecdbdc8 "%s%s%s:%u: %s%sAssertion `%s' failed.\\n%n", assertion=assertion@entry=0x7f5bc1336419 "(i >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType()) && \\"Calling a function with a bad signature!\\"", file=file@entry=0x7f5bc1336051 "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp", line=line@entry=299, function=function@entry=0x7f5bc13362af "void llvm::CallInst::init(llvm::FunctionType *, llvm::Value *, ArrayRef, ArrayRef, const llvm::Twine &)") at ./assert/assert.c:92 #5 0x7f5bceb763a2 in __assert_fail (assertion=0x7f5bc1336419 "(i >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType()) && \\"Calling a function with a bad signature!\\"", file=0x7f5bc1336051 "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp", line=299, function=0x7f5bc13362af "void llvm::CallInst::init(llvm::FunctionType *, llvm::Value *, ArrayRef, ArrayRef, const llvm::Twine &)") at ./assert/assert.c:101 #6 0x7f5bc110f138 in llvm::CallInst::init (this=0x557a91f3e508, FTy=0x557a91ed9ae0, Func=0x557a91f8be88, Args=..., Bundles=..., NameStr=...) at /home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp:297 #7 0x7f5bc0fa579d in llvm::CallInst::CallInst (this=0x557a91f3e508, Ty=0x557a91ed9ae0, Func=0x557a91f8be88, Args=..., Bundles=..., NameStr=..., InsertBefore=0x0) at /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1934 #8 0x7f5bc0fa538c in llvm::CallInst::Create (Ty=0x557a91ed9ae0, Func=0x557a91f8be88, Args=..., Bundles=..., NameStr=..., InsertBefore=0x0) at /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1444 #9 0x7f5bc0fa51f9 in llvm::IRBuilder::CreateCall (this=0x557a91f9c6a0, FTy=0x557a91ed9ae0, Callee=0x557a91f8be88, Args=..., Name=..., FPMathTag=0x0) at /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1669 #10 0x7f5bc100edda in llvm::IRBuilder::CreateCall (this=0x557a91f9c6a0, Callee=0x557a91f8be88, Args=..., Name=..., FPMathTag=0x0) at /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1663 #11 0x7f5bc100714e in LLVMBuildCall (B=0x557a91f9c6a0, Fn=0x557a91f8be88, Args=0x7ffde6fa0b50, NumArgs=6, Name=0x7f5bc30b648c "funccall_iocoerce_in_safe") at /home/bf/src/llvm-project-5/llvm/lib/IR/Core.cpp:2964 #12 0x7f5bc30af861 in llvm_compile_expr (state=0x557a91fbeac0) at /home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/jit/llvm/llvmjit_expr.c:1373 #13 0x557a915992db in jit_compile_expr (state=state@entry=0x557a91fbeac0) at /home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/jit/jit.c:177 #14 0x557a9123071d in ExecReadyExpr (state=state@entry=0x557a91fbeac0) at /home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/executor/execExpr.c:880 #15 0x557a912340d7 in ExecBuildProjectionInfo (targetList=0x557a91fa6b58, econtext=, slot=, parent=parent@entry=0x557a9
Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning
On Wed, Sep 27, 2023 at 8:07 PM Ashutosh Bapat wrote: > On Wed, Sep 27, 2023 at 2:30 PM Amit Langote wrote: > > + /* > > +* But the list of operator OIDs and the list of expressions may be > > +* referenced somewhere else. Do not free those. > > +*/ > > > > I don't see build_child_join_sjinfo() copying any list of OIDs, so I'm > > not sure what the comment is referring to. Also, instead of "the list > > of expressions", it might be more useful to mention semi_rhs_expr, > > because that's the only one being copied. > > list of OID is semi_operators. It's copied as is from parent > SpecialJoinInfo. But the way it's mentioned in the comment is > confusing. I have modified the prologue of function to provide a > general guidance on what can be freed and what should not be. and then > specific examples. Let me know if this is more clear. Thanks. I would've kept the notes about specific members inside the function. Also, no need to have a note for pointers that are not deep-copied to begin with, such as semi_operators. IOW, something like the following would have sufficed: @@ -1735,6 +1735,10 @@ build_child_join_sjinfo(PlannerInfo *root, SpecialJoinInfo *parent_sjinfo, /* * free_child_sjinfo_members * Free memory consumed by members of a child SpecialJoinInfo. + * + * Only members that are translated copies of their counterpart in the parent + * SpecialJoinInfo are freed here. However, members that could be referenced + * elsewhere are not freed. */ static void free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo) @@ -1755,10 +1759,7 @@ free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo) bms_free(child_sjinfo->syn_lefthand); bms_free(child_sjinfo->syn_righthand); - /* -* But the list of operator OIDs and the list of expressions may be -* referenced somewhere else. Do not free those. - */ + /* semi_rhs_exprs may be referenced, so don't free. */ } -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning
On Wed, Sep 27, 2023 at 8:07 PM Ashutosh Bapat wrote: > On Wed, Sep 27, 2023 at 2:30 PM Amit Langote wrote: > > Just out of curiosity, is their not being present in join_info_list > > problematic in some manner, such as missed optimization opportunities > > for child joins? I noticed there is a loop over join_info_list in > > add_paths_to_join_rel(), which does get called for child joinrels. I > > know this a bit off-topic for the thread, but thought to ask in case > > you've already thought about it. > > The function has a comment and code to take care of this at the very beginning > /* > * PlannerInfo doesn't contain the SpecialJoinInfos created for joins > * between child relations, even if there is a SpecialJoinInfo node for > * the join between the topmost parents. So, while calculating Relids set > * representing the restriction, consider relids of topmost parent of > * partitions. > */ > if (joinrel->reloptkind == RELOPT_OTHER_JOINREL) > joinrelids = joinrel->top_parent_relids; > else > joinrelids = joinrel->relids; Ah, that's accounted for. Thanks. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning
Hi Ashutosh, On Thu, Sep 21, 2023 at 1:20 PM Ashutosh Bapat wrote: > On Thu, Sep 21, 2023 at 6:37 AM Amit Langote wrote: > > On Wed, Sep 20, 2023 at 10:24 PM Ashutosh Bapat > > wrote: > > > On Wed, Sep 20, 2023 at 5:24 PM Amit Langote > > > wrote: > > > > Just one comment on 0003: > > > > > > > > + /* > > > > +* Dummy SpecialJoinInfos do not have any translated fields and > > > > hence have > > > > +* nothing to free. > > > > +*/ > > > > + if (child_sjinfo->jointype == JOIN_INNER) > > > > + return; > > > > > > > > Should this instead be Assert(child_sjinfo->jointype != JOIN_INNER)? > > > > > > try_partitionwise_join() calls free_child_sjinfo_members() > > > unconditionally i.e. it will be called even SpecialJoinInfos of INNER > > > join. Above condition filters those out early. In fact if we convert > > > into an Assert, in a production build the rest of the code will free > > > Relids which are referenced somewhere else causing dangling pointers. > > > > You're right. I hadn't realized that the parent SpecialJoinInfo > > passed to try_partitionwise_join() might itself be a dummy. I guess I > > should've read the whole thing before commenting. > > Maybe there's something to improve in terms of readability, I don't know. Here are some comments. Please merge 0003 into 0002. + /* +* But the list of operator OIDs and the list of expressions may be +* referenced somewhere else. Do not free those. +*/ I don't see build_child_join_sjinfo() copying any list of OIDs, so I'm not sure what the comment is referring to. Also, instead of "the list of expressions", it might be more useful to mention semi_rhs_expr, because that's the only one being copied. The comment for SpecialJoinInfo mentions that they are stored in PlannerInfo.join_info_list, but the child SpecialJoinInfos used by partitionwise joins are not, which I noticed has not been mentioned anywhere. Should we make a note of that in the SpecialJoinInfo's comment? Just out of curiosity, is their not being present in join_info_list problematic in some manner, such as missed optimization opportunities for child joins? I noticed there is a loop over join_info_list in add_paths_to_join_rel(), which does get called for child joinrels. I know this a bit off-topic for the thread, but thought to ask in case you've already thought about it. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: generic plans and "initial" pruning
On Wed, Sep 6, 2023 at 11:20 PM Robert Haas wrote: > On Wed, Sep 6, 2023 at 5:12 AM Amit Langote wrote: > > Attached updated patches. Thanks for the review. > > I think 0001 looks ready to commit. I'm not sure that the commit > message needs to mention future patches here, since this code cleanup > seems like a good idea regardless, but if you feel otherwise, fair > enough. OK, I will remove the mention of future patches. > On 0002, some questions: > > - In ExecEndLockRows, is the call to EvalPlanQualEnd a concern? i.e. > Does that function need any adjustment? I think it does with the patch as it stands. It needs to have an early exit at the top if parentestate is NULL, which it would be if EvalPlanQualInit() wasn't called from an ExecInit*() function. Though, as I answer below your question as to whether there is actually any need to interrupt all of the ExecInit*() routines, nothing needs to change in ExecEndLockRows(). > - In ExecEndMemoize, should there be a null-test around > MemoryContextDelete(node->tableContext) as we have in > ExecEndRecursiveUnion, ExecEndSetOp, etc.? Oops, you're right. Added. > I wonder how we feel about setting pointers to NULL after freeing the > associated data structures. The existing code isn't consistent about > doing that, and making it do so would be a fairly large change that > would bloat this patch quite a bit. On the other hand, I think it's a > good practice as a general matter, and we do do it in some ExecEnd > functions. I agree that it might be worthwhile to take the opportunity and make the code more consistent in this regard. So, I've included those changes too in 0002. > On 0003, I have some doubt about whether we really have all the right > design decisions in detail here: > > - Why have this weird rule where sometimes we return NULL and other > times the planstate? Is there any point to such a coding rule? Why not > just always return the planstate? > > - Is there any point to all of these early exit cases? For example, in > ExecInitBitmapAnd, why exit early if initialization fails? Why not > just plunge ahead and if initialization failed the caller will notice > that and when we ExecEndNode some of the child node pointers will be > NULL but who cares? The obvious disadvantage of this approach is that > we're doing a bunch of unnecessary initialization, but we're also > speeding up the common case where we don't need to abort by avoiding a > branch that will rarely be taken. I'm not quite sure what the right > thing to do is here. > > - The cases where we call ExecGetRangeTableRelation or > ExecOpenScanRelation are a bit subtler ... maybe initialization that > we're going to do later is going to barf if the tuple descriptor of > the relation isn't what we thought it was going to be. In that case it > becomes important to exit early. But if that's not actually a problem, > then we could apply the same principle here also -- don't pollute the > code with early-exit cases, just let it do its thing and sort it out > later. Do you know what the actual problems would be here if we didn't > exit early in these cases? > > - Depending on the answers to the above points, one thing we could > think of doing is put an early exit case into ExecInitNode itself: if > (unlikely(!ExecPlanStillValid(whatever)) return NULL. Maybe Andres or > someone is going to argue that that checks too often and is thus too > expensive, but it would be a lot more maintainable than having similar > checks strewn throughout the ExecInit* functions. Perhaps it deserves > some thought/benchmarking. More generally, if there's anything we can > do to centralize these checks in fewer places, I think that would be > worth considering. The patch isn't terribly large as it stands, so I > don't necessarily think that this is a critical issue, but I'm just > wondering if we can do better. I'm not even sure that it would be too > expensive to just initialize the whole plan always, and then just do > one test at the end. That's not OK if the changed tuple descriptor (or > something else) is going to crash or error out in a funny way or > something before initialization is completed, but if it's just going > to result in burning a few CPU cycles in a corner case, I don't know > if we should really care. I thought about this some and figured that adding the is-CachedPlan-still-valid tests in the following places should suffice after all: 1. In InitPlan() right after the top-level ExecInitNode() calls 2. In ExecInit*() functions of Scan nodes, right after ExecOpenScanRelation() calls CachedPlans can only become invalid because of concurrent changes to the inheritance child tables referenced in the plan. Only the following schema modifications of child tables are possible to be performed conc
Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning
On Wed, Sep 20, 2023 at 10:24 PM Ashutosh Bapat wrote: > On Wed, Sep 20, 2023 at 5:24 PM Amit Langote wrote: > > Just one comment on 0003: > > > > + /* > > +* Dummy SpecialJoinInfos do not have any translated fields and hence > > have > > +* nothing to free. > > +*/ > > + if (child_sjinfo->jointype == JOIN_INNER) > > + return; > > > > Should this instead be Assert(child_sjinfo->jointype != JOIN_INNER)? > > try_partitionwise_join() calls free_child_sjinfo_members() > unconditionally i.e. it will be called even SpecialJoinInfos of INNER > join. Above condition filters those out early. In fact if we convert > into an Assert, in a production build the rest of the code will free > Relids which are referenced somewhere else causing dangling pointers. You're right. I hadn't realized that the parent SpecialJoinInfo passed to try_partitionwise_join() might itself be a dummy. I guess I should've read the whole thing before commenting. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning
Hi Ashutosh, On Wed, Aug 16, 2023 at 2:28 PM Ashutosh Bapat wrote: > On Fri, Aug 4, 2023 at 2:11 PM Ashutosh Bapat > wrote: > > > > Attached patchset fixing those. > > 0001 - patch to report planning memory, with to explain.out regression > > output fix. We may consider committing this as well. > > 0002 - with your comment addressed above. > > 0003 - Added this patch for handling SpecialJoinInfos for inner joins. > These SpecialJoinInfos are created on the fly for parent joins. They > can be created on the fly for child joins as well without requiring > any translations. Thus they do not require any new memory. This patch > is intended to be merged into 0002 finally. I read this thread and have been reading the latest patch. At first glance, it seems quite straightforward to me. I agree with Richard that pfree()'ing 4 bitmapsets may not be a lot of added overhead. I will study the patch a bit more. Just one comment on 0003: + /* +* Dummy SpecialJoinInfos do not have any translated fields and hence have +* nothing to free. +*/ + if (child_sjinfo->jointype == JOIN_INNER) + return; Should this instead be Assert(child_sjinfo->jointype != JOIN_INNER)? -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Tue, Sep 19, 2023 at 9:00 PM Amit Langote wrote:
> On Tue, Sep 19, 2023 at 7:37 PM jian he wrote:
> > ---
> > https://www.postgresql.org/docs/current/extend-type-system.html#EXTEND-TYPES-POLYMORPHIC
> > >> When the return value of a function is declared as a polymorphic type,
> > >> there must be at least one argument position that is also
> > >> polymorphic, and the actual data type(s) supplied for the polymorphic
> > >> arguments determine the actual result type for that call.
> >
> > select json_query(jsonb'{"a":[{"a":[2,3]},{"a":[4,5]}]}','$.a[*].a?(@<=3)'
> > returning anyrange);
> > should fail. Now it returns NULL. Maybe we can validate it in
> > transformJsonFuncExpr?
> > ---
>
> I'm not sure whether we should make the parser complain about the
> weird types being specified in RETURNING.
Sleeping over this, maybe adding the following to
transformJsonOutput() does make sense?
+ if (get_typtype(ret->typid) == TYPTYPE_PSEUDO)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("returning pseudo-types is not supported in
SQL/JSON functions"));
+
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Tue, Sep 19, 2023 at 7:37 PM jian he wrote:
> On Mon, Sep 18, 2023 at 7:51 PM Erik Rijkers wrote:
> >
> > and FYI: None of these crashes occur when I leave off the 'WITH WRAPPER'
> > clause.
> >
> > >
> > > Erik
> > >
>
> if specify with wrapper, then default behavior is keep quotes, so
> jexpr->omit_quotes will be false, which make val_string NULL.
> in ExecEvalJsonExprCoercion: InputFunctionCallSafe, val_string is
> NULL, flinfo->fn_strict is true, it will return: *op->resvalue =
> (Datum) 0. but at the same time *op->resnull is still false!
>
> if not specify with wrapper, then JsonPathQuery will return NULL.
> (because after apply the path_expression, cannot multiple SQL/JSON
> items)
>
> select json_query(jsonb'{"a":[{"a":3},{"a":[4,5]}]}','$.a[*].a?(@<=3)'
> returning int4range);
> also make server crash, because default is KEEP QUOTES, so in
> ExecEvalJsonExprCoercion jexpr->omit_quotes will be false.
> val_string will be NULL again as mentioned above.
That's right.
> another funny case:
> create domain domain_int4range int4range;
> select json_query(jsonb'{"a":[{"a":[2,3]},{"a":[4,5]}]}','$.a[*].a?(@<=3)'
> returning domain_int4range with wrapper);
>
> should I expect it to return [2,4) ?
This is what you'll get with v16 that I just posted.
> ---
> https://www.postgresql.org/docs/current/extend-type-system.html#EXTEND-TYPES-POLYMORPHIC
> >> When the return value of a function is declared as a polymorphic type,
> >> there must be at least one argument position that is also
> >> polymorphic, and the actual data type(s) supplied for the polymorphic
> >> arguments determine the actual result type for that call.
>
> select json_query(jsonb'{"a":[{"a":[2,3]},{"a":[4,5]}]}','$.a[*].a?(@<=3)'
> returning anyrange);
> should fail. Now it returns NULL. Maybe we can validate it in
> transformJsonFuncExpr?
> ---
I'm not sure whether we should make the parser complain about the
weird types being specified in RETURNING. The NULL you get in the
above example is because of the following error:
select json_query(jsonb'{"a":[{"a":[2,3]},{"a":[4,5]}]}','$.a[*].a?(@<=3)'
returning anyrange error on error);
ERROR: JSON path expression in JSON_QUERY should return singleton
item without wrapper
HINT: Use WITH WRAPPER clause to wrap SQL/JSON item sequence into array.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
Hi Erik,
On Mon, Sep 18, 2023 at 19:09 Erik Rijkers wrote:
> Op 9/18/23 om 05:15 schreef Amit Langote:
> > On Sun, Sep 17, 2023 at 3:34 PM Erik Rijkers wrote:
> >> Op 9/14/23 om 10:14 schreef Amit Langote:
> >>>
> >>>
> >>
> >> Hi Amit,
> >>
> >> Just now I built a v14-patched server and I found this crash:
> >>
> >> select json_query(jsonb '
> >> {
> >> "arr": [
> >> {"arr": [2,3]}
> >> , {"arr": [4,5]}
> >> ]
> >> }'
> >> , '$.arr[*].arr ? (@ <= 3)' returning anyarray WITH WRAPPER)
> --crash
> >> ;
> >> server closed the connection unexpectedly
> >> This probably means the server terminated abnormally
> >> before or while processing the request.
> >> connection to server was lost
> >
> > Thanks for the report.
> >
> > Attached updated version fixes the crash, but you get an error as is
> > to be expected:
> >
> > select json_query(jsonb '
> > {
> > "arr": [
> > {"arr": [2,3]}
> > , {"arr": [4,5]}
> > ]
> > }'
> > , '$.arr[*].arr ? (@ <= 3)' returning anyarray WITH WRAPPER);
> > ERROR: cannot accept a value of type anyarray
> >
> > unlike when using int[]:
> >
> > select json_query(jsonb '
> > {
> > "arr": [
> > {"arr": [2,3]}
> > , {"arr": [4,5]}
> > ]
> > }'
> > , '$.arr[*].arr ? (@ <= 3)' returning int[] WITH WRAPPER);
> > json_query
> >
> > {2,3}
> > (1 row)
> >
>
> Thanks, Amit. Alas, there are more: for 'anyarray' I thought I'd
> substitute 'interval', 'int4range', 'int8range', and sure enough they
> all give similar crashes. Patched with v15:
>
> psql -qX -e << SQL
> select json_query(jsonb'{"a":[{"a":[2,3]},{"a":[4,5]}]}',
>'$.a[*].a?(@<=3)'returning int[] with wrapper --ok
> );
>
> select json_query(jsonb'{"a": [{"a": [2,3]}, {"a": [4,5]}]}',
>'$.a[*].a?(@<=3)'returning interval with wrapper --crash
> --'$.a[*].a?(@<=3)'returning int4range with wrapper --crash
> --'$.a[*].a?(@<=3)'returning int8range with wrapper --crash
> --'$.a[*].a?(@<=3)'returning numeric[] with wrapper --{2,3} =ok
> --'$.a[*].a?(@<=3)'returning anyarray with wrapper --fixed
> --'$.a[*].a?(@<=3)'returning anyarray --null =ok
> --'$.a[*].a?(@<=3)'returning int--null =ok
> --'$.a[*].a?(@<=3)'returning int with wrapper --error =ok
> --'$.a[*].a?(@<=3)'returning int[] with wrapper -- {2,3} =ok
> );
> SQL
> => server closed the connection unexpectedly, etc
>
> Because those first three tries gave a crash (*all three*), I'm a bit
> worried there may be many more.
>
> I am sorry to be bothering you with these somewhat idiotic SQL
> statements but I suppose somehow it needs to be made more solid.
No, thanks for your testing. I’ll look into these.
>
Re: dubious warning: FORMAT JSON has no effect for json and jsonb types
On Fri, Aug 18, 2023 at 2:59 PM Peter Eisentraut wrote:
> On 16.08.23 16:59, Merlin Moncure wrote:
> > On Wed, Aug 16, 2023 at 8:55 AM Peter Eisentraut > <mailto:pe...@eisentraut.org>> wrote:
> >
> > This warning comes from parse_expr.c transformJsonValueExpr() and is
> > triggered for example by the following test case:
> >
> > SELECT JSON_OBJECT('foo': NULL::json FORMAT JSON);
> > WARNING: FORMAT JSON has no effect for json and jsonb types
> >
> > But I don't see anything in the SQL standard that would require this
> > warning. It seems pretty clear that FORMAT JSON in this case is
> > implicit and otherwise without effect.
> >
> > Also, we don't have that warning in the output case (RETURNING json
> > FORMAT JSON).
> >
> > Anyone remember why this is here? Should we remove it?
> >
> >
> > +1 for removing, on the basis that it is not suprising, and would
> > pollute logs for most configurations.
>
> done
+1 and thanks. May have been there as a debugging aid if anything.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Tue, Aug 15, 2023 at 5:58 PM jian he wrote: > Hi. > in v11, json_query: > +The returned data_type has the > same semantics > +as for constructor functions like > json_objectagg; > +the default returned type is text. > +The ON EMPTY clause specifies the behavior if the > +path_expression yields no value at all; > the > +default when ON ERROR is not specified is > to return a > +null value. > > the default returned type is jsonb? You are correct. > Also in above quoted second last > line should be ON EMPTY ? Correct too. > Other than that, the doc looks good. Thanks for the review. I will post a new version after finishing working on a few other improvements I am working on. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: generic plans and "initial" pruning
On Fri, Aug 11, 2023 at 14:31 Amit Langote wrote: > On Wed, Aug 9, 2023 at 1:05 AM Robert Haas wrote: > > On Tue, Aug 8, 2023 at 10:32 AM Amit Langote > wrote: > > > But should ExecInitNode() subroutines return the partially initialized > > > PlanState node or NULL on detecting invalidation? If I'm > > > understanding how you think this should be working correctly, I think > > > you mean the former, because if it were the latter, ExecInitNode() > > > would end up returning NULL at the top for the root and then there's > > > nothing to pass to ExecEndNode(), so no way to clean up to begin with. > > > In that case, I think we will need to adjust ExecEndNode() subroutines > > > to add `if (node->ps.ps_ResultTupleSlot)` in the above code, for > > > example. That's something Tom had said he doesn't like very much [1]. > > > > Yeah, I understood Tom's goal as being "don't return partially > > initialized nodes." > > > > Personally, I'm not sure that's an important goal. In fact, I don't > > even think it's a desirable one. It doesn't look difficult to audit > > the end-node functions for cases where they'd fail if a particular > > pointer were NULL instead of pointing to some real data, and just > > fixing all such cases to have NULL-tests looks like purely mechanical > > work that we are unlikely to get wrong. And at least some cases > > wouldn't require any changes at all. > > > > If we don't do that, the complexity doesn't go away. It just moves > > someplace else. Presumably what we do in that case is have > > ExecInitNode functions undo any initialization that they've already > > done before returning NULL. There are basically two ways to do that. > > Option one is to add code at the point where they return early to > > clean up anything they've already initialized, but that code is likely > > to substantially duplicate whatever the ExecEndNode function already > > knows how to do, and it's very easy for logic like this to get broken > > if somebody rearranges an ExecInitNode function down the road. > > Yeah, I too am not a fan of making ExecInitNode() clean up partially > initialized nodes. > > > Option > > two is to rearrange the ExecInitNode functions now, to open relations > > or recurse at the beginning, so that we discover the need to fail > > before we initialize anything. That restricts our ability to further > > rearrange the functions in future somewhat, but more importantly, > > IMHO, it introduces more risk right now. Checking that the ExecEndNode > > function will not fail if some pointers are randomly null is a lot > > easier than checking that changing the order of operations in an > > ExecInitNode function breaks nothing. > > > > I'm not here to say that we can't do one of those things. But I think > > adding null-tests to ExecEndNode functions looks like *far* less work > > and *way* less risk. > > +1 > > > There's a second issue here, too, which is when we abort ExecInitNode > > partway through, how do we signal that? You're rightly pointing out > > here that if we do that by returning NULL, then we don't do it by > > returning a pointer to the partially initialized node that we just > > created, which means that we either need to store those partially > > initialized nodes in a separate data structure as you propose to do in > > 0001, > > > > or else we need to pick a different signalling convention. We > > could change (a) ExecInitNode to have an additional argument, bool > > *kaboom, or (b) we could make it return bool and return the node > > pointer via a new additional argument, or (c) we could put a Boolean > > flag into the estate and let the function signal failure by flipping > > the value of the flag. > > The failure can already be detected by seeing that > ExecPlanIsValid(estate) is false. The question is what ExecInitNode() > or any of its subroutines should return once it is. I think the > following convention works: > > Return partially initialized state from ExecInit* function where we > detect the invalidation after calling ExecInitNode() on a child plan, > so that ExecEndNode() can recurse to clean it up. > > Return NULL from ExecInit* functions where we detect the invalidation > after opening and locking a relation but before calling ExecInitNode() > to initialize a child plan if there's one at all. Even if we may set > things like ExprContext, TupleTableSlot fields, they are cleaned up > independently of the plan tree anyway via the cleanup called with > es_exprcontexts, es_tupleTable, respectively. I e
Re: initial pruning in parallel append
On Wed, Aug 9, 2023 at 9:48 PM Robert Haas wrote:
> On Wed, Aug 9, 2023 at 6:22 AM Amit Langote wrote:
> > > > I'm assuming it's not
> > > > too ugly if ExecInitAppend() uses IsParallelWorker() to decide whether
> > > > it should be writing to EState.es_part_prune_results or reading from
> > > > it -- the former if in the leader and the latter in a worker.
> > >
> > > I don't think that's too ugly. I mean you have to have an if statement
> > > someplace.
> >
> > Yes, that makes sense.
> >
> > It's just that I thought maybe I haven't thought hard enough about
> > options before adding a new IsParallelWorker(), because I don't find
> > too many instances of IsParallelWorker() in the generic executor code.
> > I think that's because most parallel worker-specific logic lives in
> > execParallel.c or in Exec*Worker() functions outside that file, so the
> > generic code remains parallel query agnostic as much as possible.
>
> Oh, actually, there is an issue here. IsParallelWorker() is not the
> right test. Imagine that there's a parallel query which launches some
> workers, and one of those calls a user-defined function which again
> uses parallelism, launching more workers. This may not be possible
> today, I don't really remember, but the test should be "am I a
> parallel worker with respect to this plan?" not "am I a parallel
> worker at all?".Not quite sure what the best way to code that is. If
> we could just test whether we have a ParallelWorkerContext, it would
> be easy...
I checked enough to be sure that IsParallelWorker() is reliable at the
time of ExecutorStart() / ExecInitNode() in ParallelQueryMain() in a
worker. However, ParallelWorkerContext is not available at that
point. Here's the relevant part of ParallelQueryMain():
/* Start up the executor */
queryDesc->plannedstmt->jitFlags = fpes->jit_flags;
ExecutorStart(queryDesc, fpes->eflags);
/* Special executor initialization steps for parallel workers */
queryDesc->planstate->state->es_query_dsa = area;
if (DsaPointerIsValid(fpes->param_exec))
{
char *paramexec_space;
paramexec_space = dsa_get_address(area, fpes->param_exec);
RestoreParamExecParams(paramexec_space, queryDesc->estate);
}
pwcxt.toc = toc;
pwcxt.seg = seg;
ExecParallelInitializeWorker(queryDesc->planstate, );
BTW, we do also use IsParallelWorker() in ExecGetRangeTableRelation()
which also probably only runs during ExecInitNode(), same as
ExecInitPartitionPruning() that this patch adds it to.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: initial pruning in parallel append
On Tue, Aug 8, 2023 at 11:16 PM Robert Haas wrote: > On Tue, Aug 8, 2023 at 2:58 AM Amit Langote wrote: > > Or we could consider something like the patch I mentioned in my 1st > > email. The idea there was to pass the pruning result via a separate > > channel, not the DSM chunk linked into the PlanState tree. To wit, on > > the leader side, ExecInitParallelPlan() puts the serialized > > List-of-Bitmapset into the shm_toc with a dedicated PARALLEL_KEY, > > alongside PlannedStmt, ParamListInfo, etc. The List-of-Bitmpaset is > > initialized during the leader's ExecInitNode(). On the worker side, > > ExecParallelGetQueryDesc() reads the List-of-Bitmapset string and puts > > the resulting node into the QueryDesc, that ParallelQueryMain() then > > uses to do ExecutorStart() which copies the pointer to > > EState.es_part_prune_results. ExecInitAppend() consults > > EState.es_part_prune_results and uses the Bitmapset from there, if > > present, instead of performing initial pruning. > > This also seems reasonable. > > > I'm assuming it's not > > too ugly if ExecInitAppend() uses IsParallelWorker() to decide whether > > it should be writing to EState.es_part_prune_results or reading from > > it -- the former if in the leader and the latter in a worker. > > I don't think that's too ugly. I mean you have to have an if statement > someplace. Yes, that makes sense. It's just that I thought maybe I haven't thought hard enough about options before adding a new IsParallelWorker(), because I don't find too many instances of IsParallelWorker() in the generic executor code. I think that's because most parallel worker-specific logic lives in execParallel.c or in Exec*Worker() functions outside that file, so the generic code remains parallel query agnostic as much as possible. > > If we > > are to go with this approach we will need to un-revert ec386948948c, > > which moved PartitionPruneInfo nodes out of Append/MergeAppend nodes > > to a List in PlannedStmt (copied into EState.es_part_prune_infos), > > such that es_part_prune_results mirrors es_part_prune_infos. > > The comment for the revert (which was > 5472743d9e8583638a897b47558066167cc14583) points to > https://www.postgresql.org/message-id/4191508.1674157...@sss.pgh.pa.us > as the reason, but it's not very clear to me why that email led to > this being reverted. In any event, I agree that if we go with your > idea to pass this via a separate PARALLEL_KEY, unreverting that patch > seems to make sense, because otherwise I think we don't have a fast > way to find the nodes that contain the state that we care about. OK, I've attached the unreverted patch that adds EState.es_part_prune_infos as 0001. 0002 adds EState.es_part_prune_results. Parallel query leader stores the bitmapset of initially valid subplans by performing initial pruning steps contained in a given PartitionPruneInfo into that list at the same index as the PartitionPruneInfo's index in es_part_prune_infos. ExecInitParallelPlan() serializes es_part_prune_results and stores it in the DSM. A worker initializes es_part_prune_results in its own EState by reading the leader's value from the DSM and for each PartitionPruneInfo in its own copy of EState.es_part_prune_infos, gets the set of initially valid subplans by referring to es_part_prune_results in lieu of performing initial pruning again. Should workers, as Tom says, instead do the pruning and cross-check the result to give an error if it doesn't match the leader's? The error message can't specifically point out which, though a user would at least know that they have functions in their database with wrong volatility markings. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v1-0001-Move-PartitioPruneInfo-out-of-plan-nodes-into-Pla.patch Description: Binary data v1-0002-Share-initial-pruning-result-between-parallel-que.patch Description: Binary data
Re: generic plans and "initial" pruning
On Tue, Aug 8, 2023 at 12:36 AM Robert Haas wrote: > On Thu, Aug 3, 2023 at 4:37 AM Amit Langote wrote: > > Here's a patch set where the refactoring to move the ExecutorStart() > > calls to be closer to GetCachedPlan() (for the call sites that use a > > CachedPlan) is extracted into a separate patch, 0002. Its commit > > message notes an aspect of this refactoring that I feel a bit nervous > > about -- needing to also move the CommandCounterIncrement() call from > > the loop in PortalRunMulti() to PortalStart() which now does > > ExecutorStart() for the PORTAL_MULTI_QUERY case. > > I spent some time today reviewing 0001. Here are a few thoughts and > notes about things that I looked at. Thanks for taking a look at this. > First, I wondered whether it was really adequate for ExecEndPlan() to > just loop over estate->es_plan_nodes and call it good. Put > differently, is it possible that we could ever have more than one > relevant EState, say for a subplan or an EPQ execution or something, > so that this loop wouldn't cover everything? I found nothing to make > me think that this is a real danger. Check. > Second, I wondered whether the ordering of cleanup operations could be > an issue. Right now, a node can position cleanup code before, after, > or both before and after recursing to child nodes, whereas with this > design change, the cleanup code will always be run before recursing to > child nodes. Because a node is appended to es_planstate_nodes at the end of ExecInitNode(), child nodes get added before their parent nodes. So the children are cleaned up first. > Here, I think we have problems. Both ExecGather and > ExecEndGatherMerge intentionally clean up the children before the > parent, so that the child shutdown happens before > ExecParallelCleanup(). Based on the comment and commit > acf555bc53acb589b5a2827e65d655fa8c9adee0, this appears to be > intentional, and you can sort of see why from looking at the stuff > that happens in ExecParallelCleanup(). If the instrumentation data > vanishes before the child nodes have a chance to clean things up, > maybe EXPLAIN ANALYZE won't reflect that instrumentation any more. If > the DSA vanishes, maybe we'll crash if we try to access it. If we > actually reach DestroyParallelContext(), we're just going to start > killing the workers. None of that sounds like what we want. > > The good news, of a sort, is that I think this might be the only case > of this sort of problem. Most nodes recurse at the end, after doing > all the cleanup, so the behavior won't change. Moreover, even if it > did, most cleanup operations look pretty localized -- they affect only > the node itself, and not its children. A somewhat interesting case is > nodes associated with subplans. Right now, because of the coding of > ExecEndPlan, nodes associated with subplans are all cleaned up at the > very end, after everything that's not inside of a subplan. But with > this change, they'd get cleaned up in the order of initialization, > which actually seems more natural, as long as it doesn't break > anything, which I think it probably won't, since as I mention in most > cases node cleanup looks quite localized, i.e. it doesn't care whether > it happens before or after the cleanup of other nodes. > > I think something will have to be done about the parallel query stuff, > though. I'm not sure exactly what. It is a little weird that Gather > and Gather Merge treat starting and killing workers as a purely > "private matter" that they can decide to handle without the executor > overall being very much aware of it. So maybe there's a way that some > of the cleanup logic here could be hoisted up into the general > executor machinery, that is, first end all the nodes, and then go > back, and end all the parallelism using, maybe, another list inside of > the estate. However, I think that the existence of ExecShutdownNode() > is a complication here -- we need to make sure that we don't break > either the case where that happen before overall plan shutdown, or the > case where it doesn't. Given that children are closed before parent, the order of operations in ExecEndGather[Merge] is unchanged. > Third, a couple of minor comments on details of how you actually made > these changes in the patch set. Personally, I would remove all of the > "is closed separately" comments that you added. I think it's a > violation of the general coding principle that you should make the > code look like it's always been that way. Sure, in the immediate > future, people might wonder why you don't need to recurse, but 5 or 10 > years from now that's just going to be clutter. Second, in the cases > where the ExecEndNode functions end up completely empty, I would &
Re: initial pruning in parallel append
On Tue, Aug 8, 2023 at 12:53 AM Robert Haas wrote:
> On Mon, Aug 7, 2023 at 10:25 AM Amit Langote wrote:
> > Note we’re talking here about “initial” pruning that occurs during
> > ExecInitNode(). Workers are only launched during ExecGather[Merge]() which
> > thereafter do ExecInitNode() on their copy of the the plan tree. So if we
> > are to pass the pruning results for cross-checking, it will have to be from
> > the leader to workers.
>
> That doesn't seem like a big problem because there aren't many node
> types that do pruning, right? I think we're just talking about Append
> and MergeAppend, or something like that, right?
MergeAppend can't be parallel-aware atm, so only Append.
> You just need the
> ExecWhateverEstimate function to budget some DSM space to store the
> information, which can basically just be a bitmap over the set of
> child plans, and the ExecWhateverInitializeDSM copies the information
> into that DSM space, and ExecWhateverInitializeWorker() copies the
> information from the shared space back into the local node (or maybe
> just points to it, if the representation is sufficiently compatible).
> I feel like this is an hour or two's worth of coding, unless I'm
> missing something, and WAY easier than trying to reason about what
> happens if expression evaluation isn't as stable as we'd like it to
> be.
OK, I agree that we'd better share the pruning result between the
leader and workers.
I hadn't thought about putting the pruning result into Append's DSM
(ParallelAppendState), which is what you're describing IIUC. I looked
into it, though I'm not sure if it can be made to work given the way
things are on the worker side, or at least not without some
reshuffling of code in ParallelQueryMain(). The pruning result will
have to be available in ExecInitAppend, but because the worker reads
the DSM only after finishing the plan tree initialization, it won't.
Perhaps, we can integrate ExecParallelInitializeWorker()'s
responsibilities into ExecutorStart() / ExecInitNode() somehow?
So change the ordering of the following code in ParallelQueryMain():
/* Start up the executor */
queryDesc->plannedstmt->jitFlags = fpes->jit_flags;
ExecutorStart(queryDesc, fpes->eflags);
/* Special executor initialization steps for parallel workers */
queryDesc->planstate->state->es_query_dsa = area;
if (DsaPointerIsValid(fpes->param_exec))
{
char *paramexec_space;
paramexec_space = dsa_get_address(area, fpes->param_exec);
RestoreParamExecParams(paramexec_space, queryDesc->estate);
}
pwcxt.toc = toc;
pwcxt.seg = seg;
ExecParallelInitializeWorker(queryDesc->planstate, );
Looking inside ExecParallelInitializeWorker():
static bool
ExecParallelInitializeWorker(PlanState *planstate, ParallelWorkerContext *pwcxt)
{
if (planstate == NULL)
return false;
switch (nodeTag(planstate))
{
case T_SeqScanState:
if (planstate->plan->parallel_aware)
ExecSeqScanInitializeWorker((SeqScanState *) planstate, pwcxt);
I guess that'd mean putting the if (planstate->plan->parallel_aware)
block seen here at the end of ExecInitSeqScan() and so on.
Or we could consider something like the patch I mentioned in my 1st
email. The idea there was to pass the pruning result via a separate
channel, not the DSM chunk linked into the PlanState tree. To wit, on
the leader side, ExecInitParallelPlan() puts the serialized
List-of-Bitmapset into the shm_toc with a dedicated PARALLEL_KEY,
alongside PlannedStmt, ParamListInfo, etc. The List-of-Bitmpaset is
initialized during the leader's ExecInitNode(). On the worker side,
ExecParallelGetQueryDesc() reads the List-of-Bitmapset string and puts
the resulting node into the QueryDesc, that ParallelQueryMain() then
uses to do ExecutorStart() which copies the pointer to
EState.es_part_prune_results. ExecInitAppend() consults
EState.es_part_prune_results and uses the Bitmapset from there, if
present, instead of performing initial pruning. I'm assuming it's not
too ugly if ExecInitAppend() uses IsParallelWorker() to decide whether
it should be writing to EState.es_part_prune_results or reading from
it -- the former if in the leader and the latter in a worker. If we
are to go with this approach we will need to un-revert ec386948948c,
which moved PartitionPruneInfo nodes out of Append/MergeAppend nodes
to a List in PlannedStmt (copied into EState.es_part_prune_infos),
such that es_part_prune_results mirrors es_part_prune_infos.
Thoughts?
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: initial pruning in parallel append
On Mon, Aug 7, 2023 at 22:29 Tom Lane wrote: > Robert Haas writes: > > ... Second, we've generally made a > > decision up until now that we don't want to have a hard dependency on > > stable functions actually being stable. If they aren't, and for > > example you're using index expressions, your queries may return wrong > > answers, but you won't get weird internal error messages, and you > > won't get a crash. I think the bar for this feature is the same. > > Yeah, I agree --- wrong answers may be acceptable in such a case, but > crashes or unintelligible error messages aren't. There are practical > reasons for being tolerant here, notably that it's not that easy > for users to get their volatility markings right. > > In the case at hand, I think that means that allowing workers to do > pruning would require hardening the parallel append code against the > situation where their pruning results vary. Maybe, instead of passing > the pruning results *down*, we could pass them *up* to the leader and > then throw an error if they're different? Note we’re talking here about “initial” pruning that occurs during ExecInitNode(). Workers are only launched during ExecGather[Merge]() which thereafter do ExecInitNode() on their copy of the the plan tree. So if we are to pass the pruning results for cross-checking, it will have to be from the leader to workers. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
Hi, On Fri, Aug 4, 2023 at 19:01 Erik Rijkers wrote: > Op 7/21/23 om 12:33 schreef Amit Langote: > > > > Thanks for taking a look. > > > > Hi Amit, > > Is there any chance to rebase the outstanding SQL/JSON patches, (esp. > json_query)? Yes, working on it. Will post a WIP shortly. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
Hello, On Thu, Jul 27, 2023 at 6:36 PM Shinoda, Noriyoshi (HPE Services Japan - FSIP) wrote: > Hi, > Thank you for developing such a great feature. The attached patch formats the > documentation like any other function definition: > - Added right parenthesis to json function calls. > - Added to json functions. > - Added a space to the 'expression' part of the json_scalar function. > - Added a space to the 'expression' part of the json_serialize function. Thanks for checking and the patch. Will push shortly. > It seems that the three functions added this time do not have tuples in the > pg_proc catalog. Is it unnecessary? Yes. These are not functions that get pg_proc entries, but SQL constructs that *look like* functions, similar to XMLEXISTS(), etc. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?
Hello,
On Fri, Jul 21, 2023 at 5:05 AM Farias de Oliveira
wrote:
>
> Hello,
>
> Thank you for the help guys and I'm so sorry for my late response. Indeed,
> the error relies on the ResultRelInfo. In GetResultRTEPermissionInfo()
> function, it does a checking on the relinfo->ri_RootResultRelInfo variable. I
> believe that it should go inside this scope:
>
>
> if (relinfo->ri_RootResultRelInfo)
> {
> /*
> * For inheritance child result relations (a partition routing target
> * of an INSERT or a child UPDATE target), this returns the root
> * parent's RTE to fetch the RTEPermissionInfo because that's the only
> * one that has one assigned.
> */
> rti = relinfo->ri_RootResultRelInfo->ri_RangeTableIndex;
> }
>
> The relinfo contains:
>
> {type = T_ResultRelInfo, ri_RangeTableIndex = 5, ri_RelationDesc =
> 0x7f44e3308cc8, ri_NumIndices = 0, ri_IndexRelationDescs = 0x0,
> ri_IndexRelationInfo = 0x0, ri_RowIdAttNo = 0,
> ri_extraUpdatedCols = 0x0, ri_projectNew = 0x0, ri_newTupleSlot = 0x0,
> ri_oldTupleSlot = 0x0, ri_projectNewInfoValid = false, ri_TrigDesc = 0x0,
> ri_TrigFunctions = 0x0,
> ri_TrigWhenExprs = 0x0, ri_TrigInstrument = 0x0, ri_ReturningSlot = 0x0,
> ri_TrigOldSlot = 0x0, ri_TrigNewSlot = 0x0, ri_FdwRoutine = 0x0, ri_FdwState
> = 0x0,
> ri_usesFdwDirectModify = false, ri_NumSlots = 0, ri_NumSlotsInitialized =
> 0, ri_BatchSize = 0, ri_Slots = 0x0, ri_PlanSlots = 0x0, ri_WithCheckOptions
> = 0x0,
> ri_WithCheckOptionExprs = 0x0, ri_ConstraintExprs = 0x0, ri_GeneratedExprsI
> = 0x0, ri_GeneratedExprsU = 0x0, ri_NumGeneratedNeededI = 0,
> ri_NumGeneratedNeededU = 0,
> ri_returningList = 0x0, ri_projectReturning = 0x0,
> ri_onConflictArbiterIndexes = 0x0, ri_onConflict = 0x0, ri_matchedMergeAction
> = 0x0, ri_notMatchedMergeAction = 0x0,
> ri_PartitionCheckExpr = 0x0, ri_ChildToRootMap = 0x0,
> ri_ChildToRootMapValid = false, ri_RootToChildMap = 0x0,
> ri_RootToChildMapValid = false, ri_RootResultRelInfo = 0x0,
> ri_PartitionTupleSlot = 0x0, ri_CopyMultiInsertBuffer = 0x0,
> ri_ancestorResultRels = 0x0}
>
> Since relinfo->ri_RootResultRelInfo = 0x0, the rti will have no value and
> Postgres will interpret that the ResultRelInfo must've been created only for
> filtering triggers and the relation is not being inserted into.
> The relinfo variable is created with the create_entity_result_rel_info()
> function:
>
> ResultRelInfo *create_entity_result_rel_info(EState *estate, char *graph_name,
> char *label_name)
> {
> RangeVar *rv;
> Relation label_relation;
> ResultRelInfo *resultRelInfo;
>
> ParseState *pstate = make_parsestate(NULL);
>
> resultRelInfo = palloc(sizeof(ResultRelInfo));
>
> if (strlen(label_name) == 0)
> {
> rv = makeRangeVar(graph_name, AG_DEFAULT_LABEL_VERTEX, -1);
> }
> else
> {
> rv = makeRangeVar(graph_name, label_name, -1);
> }
>
> label_relation = parserOpenTable(pstate, rv, RowExclusiveLock);
>
> // initialize the resultRelInfo
> InitResultRelInfo(resultRelInfo, label_relation,
> list_length(estate->es_range_table), NULL,
> estate->es_instrument);
>
> // open the parse state
> ExecOpenIndices(resultRelInfo, false);
>
> free_parsestate(pstate);
>
> return resultRelInfo;
> }
>
> In this case, how can we get the relinfo->ri_RootResultRelInfo to store the
> appropriate data?
Your function doesn't seem to have access to the ModifyTableState
node, so setting ri_RootResultRelInfo to the correct ResultRelInfo
node does not seem doable.
As I suggested in my previous reply, please check if passing 0 (not
list_length(estate->es_range_table)) for the 3rd argument
InitResultRelInfo() fixes the problem and gives the correct result.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Fri, Jul 21, 2023 at 7:33 PM Amit Langote wrote: > On Fri, Jul 21, 2023 at 1:02 AM Alvaro Herrera > wrote: > > On 2023-Jul-21, Amit Langote wrote: > > > > > I’m thinking of pushing 0001 and 0002 tomorrow barring objections. > > > > 0001 looks reasonable to me. I think you asked whether to squash that > > one with the other bugfix commit for the same code that you already > > pushed to master; I think there's no point in committing as separate > > patches, because the first one won't show up in the git_changelog output > > as a single entity with the one in 16, so it'll just be additional > > noise. > > OK, pushed 0001 to HEAD and b6e1157e7d + 0001 to 16. > > > I've looked at 0002 at various points in time and I think it looks > > generally reasonable. I think your removal of a couple of newlines > > (where originally two appear in sequence) is unwarranted; that the name > > to_json[b]_worker is ugly for exported functions (maybe "datum_to_json" > > would be better, or you may have better ideas); > > Went with datum_to_json[b]. Created a separate refactoring patch for > this, attached as 0001. > > Created another refactoring patch for the hunks related to renaming of > a nonterminal in gram.y, attached as 0002. > > > and that the omission of > > the stock comment in the new stanzas in FigureColnameInternal() is > > strange. > > Yes, fixed. > > > But I don't have anything serious. Do add some ecpg tests ... > > Added. > > > Also, remember to pgindent and bump catversion, if you haven't already. > > Will do. Wasn't sure myself whether the catversion should be bumped, > but I suppose it must be because ruleutils.c has changed. > > Attaching latest patches. Will push 0001, 0002, and 0003 on Monday to > avoid worrying about the buildfarm on a Friday evening. And pushed. Will post the remaining patches after addressing jian he's comments. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns
Hello,
On Thu, Jul 20, 2023 at 4:06 PM Zhang Mingli wrote:
>
> Hi,
>
> On Jul 7, 2023 at 18:00 +0800, Damir Belyalov , wrote:
>
>
> V2 patch still have some errors when apply file doc/src/sgml/ref/copy.sgml,
> rebased and fixed it in V3 path.
> Thanks a lot for review.
>
> I have updated https://commitfest.postgresql.org/43/3896/ to staus Ready for
> Committer, thanks again.
I've looked at this patch and it looks mostly fine, though I do not
intend to commit it myself; perhaps Andrew will.
A few minor things to improve:
+ If * is specified, it will be applied in all columns.
...
+ If * is specified, it will be applied in all columns.
Please write "it will be applied in" as "the option will be applied to".
+ boolforce_notnull_all; /* FORCE_NOT_NULL * */
...
+ boolforce_null_all; /* FORCE_NULL * */
Like in the comment for force_quote, please add a "?" after * in the
above comments.
+ if (cstate->opts.force_notnull_all)
+ MemSet(cstate->opts.force_notnull_flags, true, num_phys_attrs
* sizeof(bool));
...
+ if (cstate->opts.force_null_all)
+ MemSet(cstate->opts.force_null_flags, true, num_phys_attrs *
sizeof(bool));
While I am not especially opposed to using this 1-line variant to set
the flags array, it does mean that there are now different styles
being used for similar code, because force_quote_flags uses a for
loop:
if (cstate->opts.force_quote_all)
{
int i;
for (i = 0; i < num_phys_attrs; i++)
cstate->opts.force_quote_flags[i] = true;
}
Perhaps we could fix the inconsistency by changing the force_quote_all
code to use MemSet() too. I'll defer whether to do that to Andrew's
judgement.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Thu, Jul 20, 2023 at 17:19 Amit Langote wrote: > On Wed, Jul 19, 2023 at 5:17 PM Amit Langote > wrote: > > On Wed, Jul 19, 2023 at 12:53 AM Alvaro Herrera > wrote: > > > On 2023-Jul-18, Amit Langote wrote: > > > > > > > Attached updated patches. In 0002, I removed the mention of the > > > > RETURNING clause in the JSON(), JSON_SCALAR() documentation, which I > > > > had forgotten to do in the last version which removed its support in > > > > code. > > > > > > > I think 0001 looks ready to go. Alvaro? > > > > > > It looks reasonable to me. > > > > Thanks for taking another look. > > > > I will push this tomorrow. > > Pushed. > > > > > Also, I've been wondering if it isn't too late to apply the following > > > > to v16 too, so as to make the code look similar in both branches: > > > > > > Hmm. > > > > > > > 785480c953 Pass constructName to transformJsonValueExpr() > > > > > > I think 785480c953 can easily be considered a bugfix on 7081ac46ace8, > so > > > I agree it's better to apply it to 16. > > > > OK. > > Pushed to 16. > > > > > b6e1157e7d Don't include CaseTestExpr in JsonValueExpr.formatted_expr > > > > > > I feel a bit uneasy about this one. It seems to assume that > > > formatted_expr is always set, but at the same time it's not obvious > that > > > it is. (Maybe this aspect just needs some more commentary). > > > > Hmm, I agree that the comments about formatted_expr could be improved > > further, for which I propose the attached. Actually, staring some > > more at this, I'm inclined to change makeJsonValueExpr() to allow > > callers to pass it the finished 'formatted_expr' rather than set it by > > themselves. > > > > > I agree > > > that it would be better to make both branches identical, because if > > > there's a problem, we are better equipped to get a fix done to both. > > > > > > As for the removal of makeCaseTestExpr(), I agree -- of the six callers > > > of makeNode(CastTestExpr), only two of them would be able to use the > new > > > function, so it doesn't look of general enough usefulness. > > > > OK, so you agree with back-patching this one too, though perhaps only > > after applying something like the aforementioned patch. > > I looked at this some more and concluded that it's fine to think that > all JsonValueExpr nodes leaving the parser have their formatted_expr > set. I've updated the commentary some more in the patch attached as > 0001. > > Rebased SQL/JSON patches also attached. I've fixed the JSON_TABLE > syntax synopsis in the documentation as mentioned in my other email. I’m thinking of pushing 0001 and 0002 tomorrow barring objections. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
Hello,
On Thu, Jul 20, 2023 at 10:35 AM jian he wrote:
> On Tue, Jul 18, 2023 at 5:11 PM Amit Langote wrote:
> > > Op 7/17/23 om 07:00 schreef jian he:
> > > > hi.
> > > > seems there is no explanation about, json_api_common_syntax in
> > > > functions-json.html
> > > >
> > > > I can get json_query full synopsis from functions-json.html as follows:
> > > > json_query ( context_item, path_expression [ PASSING { value AS
> > > > varname } [, ...]] [ RETURNING data_type [ FORMAT JSON [ ENCODING UTF8
> > > > ] ] ] [ { WITHOUT | WITH { CONDITIONAL | [UNCONDITIONAL] } } [ ARRAY ]
> > > > WRAPPER ] [ { KEEP | OMIT } QUOTES [ ON SCALAR STRING ] ] [ { ERROR |
> > > > NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression } ON EMPTY ]
> > > > [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > > > ON ERROR ])
> > > >
> > > > seems doesn't have a full synopsis for json_table? only partial one
> > > > by one explanation.
> >
> > I looked through the history of the docs portion of the patch and it
> > looks like the synopsis for JSON_TABLE(...) used to be there but was
> > taken out during one of the doc reworks [1].
> >
> > I've added it back in the patch as I agree that it would help to have
> > it. Though, I am not totally sure where I've put it is the right
> > place for it. JSON_TABLE() is a beast that won't fit into the table
> > that JSON_QUERY() et al are in, so maybe that's how it will have to
> > be? I have no better idea.
>
> attached screenshot render json_table syntax almost plain html. It looks fine.
Thanks for checking.
> based on syntax, then I am kind of confused with following 2 cases:
> --1
> SELECT * FROM JSON_TABLE(jsonb '1', '$'
> COLUMNS (a int PATH 'strict $.a' default 1 ON EMPTY default 2 on error)
> ERROR ON ERROR) jt;
>
> --2
> SELECT * FROM JSON_TABLE(jsonb '1', '$'
> COLUMNS (a int PATH 'strict $.a' default 1 ON EMPTY default 2 on error))
> jt;
>
> the first one should yield syntax error?
No. Actually, the synopsis missed the optional ON ERROR clause that
can appear after COLUMNS(...). Will fix it.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Wed, Jul 19, 2023 at 5:17 PM Amit Langote wrote: > On Wed, Jul 19, 2023 at 12:53 AM Alvaro Herrera > wrote: > > On 2023-Jul-18, Amit Langote wrote: > > > b6e1157e7d Don't include CaseTestExpr in JsonValueExpr.formatted_expr > > > > I feel a bit uneasy about this one. It seems to assume that > > formatted_expr is always set, but at the same time it's not obvious that > > it is. (Maybe this aspect just needs some more commentary). > > Hmm, I agree that the comments about formatted_expr could be improved > further, for which I propose the attached. Actually, staring some > more at this, I'm inclined to change makeJsonValueExpr() to allow > callers to pass it the finished 'formatted_expr' rather than set it by > themselves. Hmm, after looking some more, it may not be entirely right that formatted_expr is always set in the code paths that call transformJsonValueExpr(). Will look at this some more tomorrow. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Wed, Jul 19, 2023 at 12:53 AM Alvaro Herrera wrote: > On 2023-Jul-18, Amit Langote wrote: > > > Attached updated patches. In 0002, I removed the mention of the > > RETURNING clause in the JSON(), JSON_SCALAR() documentation, which I > > had forgotten to do in the last version which removed its support in > > code. > > > I think 0001 looks ready to go. Alvaro? > > It looks reasonable to me. Thanks for taking another look. I will push this tomorrow. > > Also, I've been wondering if it isn't too late to apply the following > > to v16 too, so as to make the code look similar in both branches: > > Hmm. > > > 785480c953 Pass constructName to transformJsonValueExpr() > > I think 785480c953 can easily be considered a bugfix on 7081ac46ace8, so > I agree it's better to apply it to 16. OK. > > b6e1157e7d Don't include CaseTestExpr in JsonValueExpr.formatted_expr > > I feel a bit uneasy about this one. It seems to assume that > formatted_expr is always set, but at the same time it's not obvious that > it is. (Maybe this aspect just needs some more commentary). Hmm, I agree that the comments about formatted_expr could be improved further, for which I propose the attached. Actually, staring some more at this, I'm inclined to change makeJsonValueExpr() to allow callers to pass it the finished 'formatted_expr' rather than set it by themselves. > I agree > that it would be better to make both branches identical, because if > there's a problem, we are better equipped to get a fix done to both. > > As for the removal of makeCaseTestExpr(), I agree -- of the six callers > of makeNode(CastTestExpr), only two of them would be able to use the new > function, so it doesn't look of general enough usefulness. OK, so you agree with back-patching this one too, though perhaps only after applying something like the aforementioned patch. Just to be sure, would the good practice in this case be to squash the fixup patch into b6e1157e7d before back-patching? -- Thanks, Amit Langote EDB: http://www.enterprisedb.com 0001-Code-review-for-commit-b6e1157e7d.patch Description: Binary data
Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?
Hello,
On Sat, Jul 15, 2023 at 4:43 AM Farias de Oliveira
wrote:
> I believe I have found something interesting that might be the root of the
> problem with RTEPermissionInfo. But I do not know how to fix it exactly. In
> AGE's code, the execution of it goes through a function called
> analyze_cypher_clause() which does the following:
>
> It ends up going inside other functions and changing it more a bit, but at
> the end of one of these functions it assigns values to some members of the
> query:
>
> query->targetList = lappend(query->targetList, tle);
> query->rtable = pstate->p_rtable;
> query->jointree = makeFromExpr(pstate->p_joinlist, NULL);
>
> I assume that here is missing the assignment of query->rteperminfos to be the
> same as pstate->p_rteperminfos, but the pstate has the following values:
>
> {pstate = {parentParseState = 0x0, p_sourcetext = 0x2b06ef0 "MATCH (n) SET
> n.i = 3", p_rtable = 0x2c6e590,
> p_rteperminfos = 0x0, p_joinexprs = 0x0, p_nullingrels = 0x0, p_joinlist
> = 0x2c6e678, p_namespace = 0x2c6e6c8,
> p_lateral_active = false, p_ctenamespace = 0x0, p_future_ctes = 0x0,
> p_parent_cte = 0x0, p_target_relation = 0x0,
> p_target_nsitem = 0x0, p_is_insert = false, p_windowdefs = 0x0,
> p_expr_kind = EXPR_KIND_NONE, p_next_resno = 3,
> p_multiassign_exprs = 0x0, p_locking_clause = 0x0, p_locked_from_parent =
> false, p_resolve_unknowns = true,
> p_queryEnv = 0x0, p_hasAggs = false, p_hasWindowFuncs = false,
> p_hasTargetSRFs = false, p_hasSubLinks = false,
> p_hasModifyingCTE = false, p_last_srf = 0x0, p_pre_columnref_hook = 0x0,
> p_post_columnref_hook = 0x0,
> p_paramref_hook = 0x0, p_coerce_param_hook = 0x0, p_ref_hook_state =
> 0x0}, graph_name = 0x2b06e50 "cypher_set",
> graph_oid = 16942, params = 0x0, default_alias_num = 0, entities =
> 0x2c6e228, property_constraint_quals = 0x0,
> exprHasAgg = false, p_opt_match = false}
>
> So changing that won't solve the issue.
Does p_rtable in this last pstate contain any RTE_RELATION entries?
If it does, p_rteperminfos being NIL looks like a bug in your code.
Actually, given the back trace of the error that you shared, I am
suspecting more of a problem in the code that generates a
ResultRelInfo pointing at the wrong RTE via its ri_RangeTableIndex.
That code should perhaps set the ri_RangeTableIndex to 0 if it doesn't
know the actual existing RTE corresponding to that result relation.
If you set it to some non-0 value, the RTE that it points to should
satisfy invariants such as having the corresponding RTEPermissionInfo
present in the rteperminfos list if necessary.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?
On Fri, Jul 14, 2023 at 7:12 AM Tom Lane wrote:
> Farias de Oliveira writes:
> > With further inspection in AGE's code, after executing the SET query,
> > it goes inside transform_cypher_clause_as_subquery() function and the
> > ParseNamespaceItem has the following values:
>
> > {p_names = 0x1205638, p_rte = 0x11edb70, p_rtindex = 1, p_perminfo =
> > 0x7f7f7f7f7f7f7f7f,
> > p_nscolumns = 0x1205848, p_rel_visible = true, p_cols_visible =
> > true, p_lateral_only = false,
> > p_lateral_ok = true}
>
> Hmm, that uninitialized value for p_perminfo is pretty suspicious.
> I see that transformFromClauseItem and buildNSItemFromLists both
> create ParseNamespaceItems without bothering to fill p_perminfo,
> while buildNSItemFromTupleDesc fills it per the caller and
> addRangeTableEntryForJoin always sets it to NULL. I think we
> ought to make the first two set it to NULL as well, because
> uninitialized fields are invariably a bad idea (even though the
> lack of valgrind complaints says that the core code is managing
> to avoid touching those fields).
Agreed, I'll go ahead and fix that.
> If we do that, is it sufficient to resolve your problem?
Hmm, I'm afraid maybe not, because if the above were the root issue,
we'd have seen a segfault and not the error the OP mentioned? I'm
thinking the issue is that their code appears to be consing up an RTE
that the core code (getRTEPermissionInfo() most likely via
markRTEForSelectPriv()) is not expecting to be called with? I would
be helpful to see a backtrace when the error occurs to be sure.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: Eliminating SPI from RI triggers - take 2
On Mon, Jul 10, 2023 at 5:27 PM Daniel Gustafsson wrote: > > On 21 Mar 2023, at 06:03, Amit Langote wrote: > > On Tue, Mar 21, 2023 at 3:54 AM Gregory Stark (as CFM) > > wrote: > >> On Mon, 17 Oct 2022 at 14:59, Robert Haas wrote: > > >>> But I think the bigger problem for this patch set is that the > >>> design-level feedback from > >>> https://www.postgresql.org/message-id/CA%2BTgmoaiTNj4DgQy42OT9JmTTP1NWcMV%2Bke0i%3D%2Ba7%3DVgnzqGXw%40mail.gmail.com > >>> hasn't really been addressed, AFAICS. ri_LookupKeyInPkRelPlanIsValid > >>> is still trivial in v7, and that still seems wrong to me. And I still > >>> don't know how we're going to avoid changing the semantics in ways > >>> that are undesirable, or even knowing precisely what we did change. If > >>> we don't have answers to those questions, then I suspect that this > >>> patch set isn't going anywhere. > >> > >> Amit, do you plan to work on this patch for this commitfest (and > >> therefore this release?). And do you think it has a realistic chance > >> of being ready for commit this month? > > > > Unfortunately, I don't think so. > > This thread has stalled with the patch not building and/or applying for a > while, so I am going to mark this Returned with Feebdback. Agreed, I was about to do so myself. I'll give this another try later in the cycle. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: remaining sql/json patches
On Fri, Jul 7, 2023 at 8:31 PM Peter Eisentraut wrote: > On 21.06.23 10:25, Amit Langote wrote: > > I realized that the patch for the "other sql/json functions" part is > > relatively straightforward and has no dependence on the "sql/json > > query functions" part getting done first. So I've made that one the > > 0001 patch. The patch I posted in the last email is now 0002, though > > it only has changes related to changing the order of the patch, so I > > decided not to change the patch version marker (v1). > > (I suggest you change the version number anyway, next time. There are > plenty of numbers available.) Will do. :) > The 0001 patch contains a change to > doc/src/sgml/keywords/sql2016-02-reserved.txt, which seems > inappropriate. The additional keywords are already listed in the 2023 > file, and they are not SQL:2016 keywords. Ah, indeed. Will remove. > Another thing, I noticed that the SQL/JSON patches in PG16 introduced > some nonstandard indentation in gram.y. I would like to apply the > attached patch to straighten this out. Sounds fine to me. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Wed, Jun 28, 2023 at 4:30 PM Amit Langote wrote: > > Hi, > > On Tue, Feb 21, 2023 at 4:12 PM Amit Langote wrote: > > On Tue, Feb 21, 2023 at 12:40 AM Tom Lane wrote: > > > Alvaro Herrera writes: > > > > On 2023-Feb-20, Amit Langote wrote: > > > >> One more thing we could try is come up with a postgres_fdw test case, > > > >> because it uses the RelOptInfo.userid value for remote-costs-based > > > >> path size estimation. But adding a test case to contrib module's > > > >> suite test a core planner change might seem strange, ;-). > > > > > > > Maybe. Perhaps adding it in a separate file there is okay? > > > > > > There is plenty of stuff in contrib module tests that is really > > > there to test core-code behavior. (You could indeed argue that > > > *all* of contrib is there for that purpose.) If it's not > > > convenient to test something without an extension, just do it > > > and don't sweat about that. > > > > OK. Attached adds a test case to postgres_fdw's suite. You can see > > that it fails without a316a3bc. > > Noticed that there's an RfC entry for this in the next CF. Here's an > updated version of the patch where I updated the comments a bit and > the commit message. > > I'm thinking of pushing this on Friday barring objections. Seeing none, I've pushed this to HEAD and 16. Marking the CF entry as committed. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
Hi, On Tue, Feb 21, 2023 at 4:12 PM Amit Langote wrote: > On Tue, Feb 21, 2023 at 12:40 AM Tom Lane wrote: > > Alvaro Herrera writes: > > > On 2023-Feb-20, Amit Langote wrote: > > >> One more thing we could try is come up with a postgres_fdw test case, > > >> because it uses the RelOptInfo.userid value for remote-costs-based > > >> path size estimation. But adding a test case to contrib module's > > >> suite test a core planner change might seem strange, ;-). > > > > > Maybe. Perhaps adding it in a separate file there is okay? > > > > There is plenty of stuff in contrib module tests that is really > > there to test core-code behavior. (You could indeed argue that > > *all* of contrib is there for that purpose.) If it's not > > convenient to test something without an extension, just do it > > and don't sweat about that. > > OK. Attached adds a test case to postgres_fdw's suite. You can see > that it fails without a316a3bc. Noticed that there's an RfC entry for this in the next CF. Here's an updated version of the patch where I updated the comments a bit and the commit message. I'm thinking of pushing this on Friday barring objections. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v2-0001-Add-a-test-case-for-a316a3bc.patch Description: Binary data
initial pruning in parallel append
Hi, In an off-list chat, Robert suggested that it might be a good idea to look more closely into $subject, especially in the context of the project of moving the locking of child tables / partitions to the ExecInitNode() phase when executing cached generic plans [1]. Robert's point is that a worker's output of initial pruning which consists of the set of child subplans (of a parallel-aware Append or MergeAppend) it considers as valid for execution may not be the same as the leader's and that of other workers. If that does indeed happen, it may confuse the Append's parallel-execution code, possibly even cause crashes, because the ParallelAppendState set up by the leader assumes a certain number and identity (?) of valid-for-execution subplans. So he suggests that initial pruning should only be done once in the leader and the result of that put in the EState for ExecInitParallelPlan() to serialize to pass down to workers. Workers would simply consume that as-is to set the valid-for-execution child subplans in its copy of AppendState, instead of doing the initial pruning again. Actually, earlier patches at [1] had implemented that mechanism (remembering the result of initial pruning and using it at a later time and place), because the earlier design there was to move the initial pruning on the nodes in a cached generic plan tree from ExecInitNode() to GetCachedPlan(). The result of initial pruning done in the latter would be passed down to and consumed in the former using what was called PartitionPruneResult nodes. Maybe that stuff could be resurrected, though I was wondering if the risk of the same initial pruning steps returning different results when performed repeatedly in *one query lifetime* aren't pretty minimal or maybe rather non-existent? I think that's because performing initial pruning steps entails computing constant and/or stable expressions and comparing them with an unchanging set of partition bound values, with comparison functions whose result is also presumed to be stable. Then there's also the step of mapping the partition indexes as they appear in the PartitionDesc to the indexes of their subplans under Append/MergeAppend using the information contained in PartitionPruneInfo (subplan_map) and the result of mapping should be immutable too. I considered that the comparison functions that match_clause_to_partition_key() obtains by calling get_opfamily_proc() may in fact not be stable, though that doesn't seem to be a worry at least with the out-of-the-box pg_amproc collection: select amproc, p.provolatile from pg_amproc, pg_proc p where amproc = p.oid and p.provolatile <> 'i'; amproc | provolatile ---+- date_cmp_timestamptz | s timestamp_cmp_timestamptz | s timestamptz_cmp_date | s timestamptz_cmp_timestamp | s pg_catalog.in_range | s (5 rows) Is it possible for a user to add a volatile procedure to pg_amproc? If that's possible, match_clause_to_partition_key() may pick one as a comparison function for pruning, because it doesn't actually check the procedure's provolatile before doing so. I'd hope not, though would like to be sure to support what I wrote above. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://commitfest.postgresql.org/43/3478/
Re: SQL/JSON revisited
Hi, On Thu, May 4, 2023 at 3:58 AM Matthias Kurz wrote: > On Wed, 3 May 2023 at 20:17, Alvaro Herrera wrote: >> I would suggest to start a new thread with updated patches, and then a new >> commitfest entry can be created with those. > > Whoever starts that new thread, please link link it here, I am keen to follow > it ;) Thanks a lot! > Thanks a lot for all your hard work btw, it's highly appreciated! Just created a new thread: https://www.postgresql.org/message-id/CA%2BHiwqE4XTdfb1nW%3DOjoy_tQSRhYt-q_kb6i5d4xcKyrLC1Nbg%40mail.gmail.com CF entry: https://commitfest.postgresql.org/43/4377/ -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Fix a typo in rewriteHandler.c
On Fri, Jun 16, 2023 at 10:25 AM Amit Langote wrote: > On Thu, Jun 15, 2023 at 5:07 PM Sho Kato (Fujitsu) > wrote: > > I've attached the patch for the following rewriteTargetView comments. > > > > s/rewriteQuery/RewriteQuery > > Good catch and thanks for the patch. Will push shortly. Done. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Fix a typo in rewriteHandler.c
Hello,
On Thu, Jun 15, 2023 at 5:07 PM Sho Kato (Fujitsu) wrote:
>
> Hi,
>
> I've attached the patch for the following rewriteTargetView comments.
>
>
> Assert(parsetree->resultRelation == new_rt_index);
>
> /*
> * For INSERT/UPDATE we must also update resnos in the targetlist to refer
> * to columns of the base relation, since those indicate the target
> * columns to be affected.
> *
> * Note that this destroys the resno ordering of the targetlist, but that
> * will be fixed when we recurse through rewriteQuery, which will invoke
> * rewriteTargetListIU again on the updated targetlist.
> */
> if (parsetree->commandType != CMD_DELETE)
> {
> foreach(lc, parsetree->targetList)
>
> s/rewriteQuery/RewriteQuery
Good catch and thanks for the patch. Will push shortly.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: obsolete filename reference in parser README
On Thu, Jun 15, 2023 at 10:48 PM Tristan Partin wrote: > Nice catch. Looks good. Thanks for checking. As just mentioned, I've pushed this moments ago. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: obsolete filename reference in parser README
On Thu, Jun 15, 2023 at 6:54 PM Amit Langote wrote: > I noticed that 2f2b18bd3f55 forgot to remove the mention of > parse_jsontable.c in src/backend/parser/README. > > Attached a patch to fix that. Will push that shortly to HEAD and v15. Pushed to HEAD only. 9853bf6ab0e that added parse_jsontable.c to the README was not back-patched. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
obsolete filename reference in parser README
Hi, I noticed that 2f2b18bd3f55 forgot to remove the mention of parse_jsontable.c in src/backend/parser/README. Attached a patch to fix that. Will push that shortly to HEAD and v15. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v1-0001-Remove-outdated-reference-to-a-removed-file.patch Description: Binary data
Re: Views no longer in rangeTabls?
On Wed, Jun 14, 2023 at 15:49 Amit Langote wrote: > On Wed, Jun 14, 2023 at 15:44 Michael Paquier wrote: > >> On Wed, Jun 14, 2023 at 02:34:56PM +0900, Amit Langote wrote: >> > This being my first commit, I intently looked to check if everything’s >> set >> > up correctly. While it seemed to have hit gitweb and GitHub, it didn’t >> > pgsql-committers for some reason. >> >> It seems to me that the email of pgsql-committers is just being held >> in moderation. Your first commit is in the tree, so this worked fine >> seen from here. Congrats! > > > Ah, did think it might be moderation. Thanks for the confirmation, > Michael. > It’s out now: https://www.postgresql.org/message-id/E1q9Gms-001h5g-8Q%40gemulon.postgresql.org -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Views no longer in rangeTabls?
On Wed, Jun 14, 2023 at 15:44 Michael Paquier wrote: > On Wed, Jun 14, 2023 at 02:34:56PM +0900, Amit Langote wrote: > > This being my first commit, I intently looked to check if everything’s > set > > up correctly. While it seemed to have hit gitweb and GitHub, it didn’t > > pgsql-committers for some reason. > > It seems to me that the email of pgsql-committers is just being held > in moderation. Your first commit is in the tree, so this worked fine > seen from here. Congrats! Ah, did think it might be moderation. Thanks for the confirmation, Michael. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Views no longer in rangeTabls?
On Wed, Jun 14, 2023 at 12:08 Amit Langote wrote: > On Tue, Jun 13, 2023 at 9:40 PM David Steele wrote: > > On 6/13/23 11:38, Amit Langote wrote: > > > On Tue, Jun 13, 2023 at 6:33 PM Alvaro Herrera < > alvhe...@alvh.no-ip.org> wrote: > > >> Note that you changed one comment from "permission checks" to > > >> "permission hecks". > > > > > > Oops, thanks for pointing that out. > > > > > > Fixed in the attached. > > > > I have done another (more careful) review of the comments and I don't > > see any other issues. > > Thanks for checking. > > Seeing no other comments, I've pushed this after rewriting the > comments that needed to be changed to mention "relkind" right after > "relid". This being my first commit, I intently looked to check if everything’s set up correctly. While it seemed to have hit gitweb and GitHub, it didn’t pgsql-committers for some reason. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Views no longer in rangeTabls?
On Tue, Jun 13, 2023 at 9:40 PM David Steele wrote: > On 6/13/23 11:38, Amit Langote wrote: > > On Tue, Jun 13, 2023 at 6:33 PM Alvaro Herrera > > wrote: > >> Note that you changed one comment from "permission checks" to > >> "permission hecks". > > > > Oops, thanks for pointing that out. > > > > Fixed in the attached. > > I have done another (more careful) review of the comments and I don't > see any other issues. Thanks for checking. Seeing no other comments, I've pushed this after rewriting the comments that needed to be changed to mention "relkind" right after "relid". -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Views no longer in rangeTabls?
On Tue, Jun 13, 2023 at 6:33 PM Alvaro Herrera wrote: > Note that you changed one comment from "permission checks" to > "permission hecks". Oops, thanks for pointing that out. Fixed in the attached. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v3-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patch Description: Binary data
Re: Views no longer in rangeTabls?
On Tue, Jun 13, 2023 at 4:44 PM David Steele wrote: > On 6/13/23 06:09, Amit Langote wrote: > > On Sat, Jun 10, 2023 at 10:27 PM Tom Lane wrote: > >> Julien Rouhaud writes: > >>> On Sat, Jun 10, 2023 at 08:56:47AM -0400, Tom Lane wrote: > >>>> - rte->relkind = 0; > >> > >>> and also handle that field in (read|out)funcs.c > >> > >> Oh, right. Ugh, that means a catversion bump. It's not like > >> we've never done that during beta, but it's kind of an annoying > >> cost for a detail as tiny as this. > > > > OK, so how about the attached? > > The patch looks good to me. I also tested it against pgAudit and > everything worked. Thanks for the review. > I decided to go with the following because I think it > is easier to read: > > /* We only care about tables/views and can ignore subqueries, etc. */ > if (!(rte->rtekind == RTE_RELATION || >(rte->rtekind == RTE_SUBQUERY && rte->relkind == RELKIND_VIEW))) > continue; It didn't occur to me so far to mention it but this could be replaced with: if (rte->perminfoindex != 0) and turn that condition into an Assert instead, like the loop over range table in ExecCheckPermissions() does. > > I considered adding Assert(relkind == RELKIND_VIEW) in all places that > > have the "rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)" > > condition, but that seemed like an overkill, so only added one in the > > #ifdef USE_ASSERT_CHECKING block in ExecCheckPermissions() that > > f75cec4fff877 added. > > This seems like a good place for the assert. I added a comment above this Assert. I'd like to push this tomorrow barring objections. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v2-0001-Retain-relkind-too-in-RTE_SUBQUERY-entries-for-vi.patch Description: Binary data
Re: Views no longer in rangeTabls?
On Sat, Jun 10, 2023 at 10:27 PM Tom Lane wrote:
> Julien Rouhaud writes:
> > On Sat, Jun 10, 2023 at 08:56:47AM -0400, Tom Lane wrote:
> >> - rte->relkind = 0;
>
> > and also handle that field in (read|out)funcs.c
>
> Oh, right. Ugh, that means a catversion bump. It's not like
> we've never done that during beta, but it's kind of an annoying
> cost for a detail as tiny as this.
OK, so how about the attached?
I considered adding Assert(relkind == RELKIND_VIEW) in all places that
have the "rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)"
condition, but that seemed like an overkill, so only added one in the
#ifdef USE_ASSERT_CHECKING block in ExecCheckPermissions() that
f75cec4fff877 added.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
From f7390a898b7e156d75372d28ea5698d2ced9795b Mon Sep 17 00:00:00 2001
From: Amit Langote
Date: Tue, 13 Jun 2023 12:52:47 +0900
Subject: [PATCH v1] Retain relkind too in RTE_SUBQUERY entries for views.
47bb9db75 modified the ApplyRetrieveRule()'s conversion of a view's
original RTE_RELATION entry into an RTE_SUBQUERY one to retain relid,
rellockmode, and perminfoindex so that the executor can lock the view
and check its permissions. It seems better to also retain
relkind for cross-checking that the exception of an
RTE_SUBQUERY entry being allowed to carry relation details only
applies to views, so do so.
Bump catversion because this changes the output format of
RTE_SUBQUERY RTEs.
Suggested-by: David Steele
Discussion: https://postgr.es/m/3953179e-9540-e5d1-a743-4bef368785b0%40pgmasters.net
---
src/backend/executor/execMain.c | 3 +++
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c| 1 +
src/backend/rewrite/rewriteHandler.c | 7 +++
src/include/catalog/catversion.h | 2 +-
src/include/nodes/parsenodes.h | 14 +++---
6 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c76fdf59ec..7aed5e7b36 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -595,6 +595,9 @@ ExecCheckPermissions(List *rangeTable, List *rteperminfos,
if (rte->perminfoindex != 0)
{
/* Sanity checks */
+ Assert(rte->rtekind == RTE_RELATION ||
+ (rte->rtekind == RTE_SUBQUERY &&
+ rte->relkind == RELKIND_VIEW));
(void) getRTEPermissionInfo(rteperminfos, rte);
/* Many-to-one mapping not allowed */
Assert(!bms_is_member(rte->perminfoindex, indexset));
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index ba00b99249..955286513d 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -513,6 +513,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
WRITE_BOOL_FIELD(security_barrier);
/* we re-use these RELATION fields, too: */
WRITE_OID_FIELD(relid);
+ WRITE_CHAR_FIELD(relkind);
WRITE_INT_FIELD(rellockmode);
WRITE_UINT_FIELD(perminfoindex);
break;
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 597e5b3ea8..a136ae1d60 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -503,6 +503,7 @@ _readRangeTblEntry(void)
READ_BOOL_FIELD(security_barrier);
/* we re-use these RELATION fields, too: */
READ_OID_FIELD(relid);
+ READ_CHAR_FIELD(relkind);
READ_INT_FIELD(rellockmode);
READ_UINT_FIELD(perminfoindex);
break;
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 0e4f76efa8..0967be762c 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1849,11 +1849,10 @@ ApplyRetrieveRule(Query *parsetree,
/*
* Clear fields that should not be set in a subquery RTE. Note that we
- * leave the relid, rellockmode, and perminfoindex fields set, so that the
- * view relation can be appropriately locked before execution and its
- * permissions checked.
+ * leave the relid, rellockmode, relkind, and perminfoindex fields set, so
+ * that the view relation can be appropriately locked before execution and
+ * its permissions checked.
*/
- rte->relkind = 0;
rte->tablesample = NULL;
rte->inh = false; /* must not be set for a subquery */
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index c784937a0e..fe70d8396d 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -57,6 +57,6 @@
*/
/* mmddN */
-#define CATALOG_VERSION_NO 202305211
+#define CATALOG_VERSION_NO 202306141
#endif
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 0ca298f5a1..786692781d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1056,13 +1056,13 @@ typedef struct RangeTblEntry
* this R
Re: Views no longer in rangeTabls?
On Sat, Jun 10, 2023 at 15:51 David Steele wrote: > On 6/9/23 19:14, Tom Lane wrote: > > David Steele writes: > >> Thank you, this was very helpful. I am able to get the expected result > >> now with: > > > >> /* We only care about tables/views and can ignore subqueries, etc. */ > >> if (!(rte->rtekind == RTE_RELATION || > >>(rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid > >> continue; > > > > Right, that matches places like add_rtes_to_flat_rtable(). > > Good to have confirmation of that, thanks! > > >> One thing, though, rte->relkind is not set for views, so I still need to > >> call get_rel_relkind(rte->relid). Not a big deal, but do you think it > >> would make sense to set rte->relkind for views? > > > > If you see "rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)", > > it's dead certain that relid refers to a view, so you could just wire > > in that knowledge. > > Yeah, that's a good trick. Even so, I wonder why relkind is not set when > relid is set? I too have been thinking that setting relkind might be a good idea, even if only as a crosscheck that only view relations can look like that in the range table. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Views no longer in rangeTabls?
Hi David, On Fri, Jun 9, 2023 at 17:28 David Steele wrote: > Hackers, > > While updating pgAudit for PG16 I found the following (from our > perspective) regression. > > In prior versions of Postgres, views were listed in rangeTabls when > ExecutorCheckPerms_hook() was called but in PG16 the views are no longer > in this list. I’m not exactly sure how pgAudit’s code is searching for view relations in the range table, but if the code involves filtering on rtekind == RTE_RELATION, then yes, such code won’t find views anymore. That’s because the rewriter no longer adds extraneous RTE_RELATION RTEs for views into the range table. Views are still there, it’s just that their RTEs are of kind RTE_SUBQUERY, but they do contain some RELATION fields like relid, rellockmode, etc. So an extension hook’s relation RTE filtering code should also consider relid, not just rtekind. I’m away from a computer atm, so I am not able to easily copy-paste an example of that from the core code, but maybe you can search for code sites that need to filter out relation RTEs from the range table. Perhaps, we are missing a comment near the hook definition mentioning this detail about views. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: The documentation for READ COMMITTED may be incomplete or wrong
Thanks for the patch.
On Fri, May 19, 2023 at 8:57 AM Tom Lane wrote:
> I wrote:
> > Debian Code Search doesn't know of any outside code touching
> > relsubs_done, so I think we are safe in dropping that code in
> > ExecScanReScan. It seems quite pointless anyway considering
> > that up to now, EvalPlanQualBegin has always zeroed the whole
> > array.
>
> Oh, belay that. What I'd forgotten is that it's possible that
> the target relation is on the inside of a nestloop, meaning that
> we might need to fetch the EPQ substitute tuple more than once.
> So there are three possible states: blocked (never return a
> tuple), ready to return a tuple, and done returning a tuple
> for this scan. ExecScanReScan needs to reset "done" to "ready",
> but not touch the "blocked" state. The attached v2 mechanizes
> that using two bool arrays.
Aha, that's clever. So ExecScanReScan() would only reset the
relsubs_done[] entry for the currently active ("unblocked") target
relation, because that would be the only one "unblocked" during a
given EvalPlanQual() invocation.
+* Initialize per-relation EPQ tuple states. Result relations, if any,
+* get marked as blocked; others as not-fetched.
Would it be helpful to clarify that "blocked" means blocked for a
given EvalPlanQual() cycle?
+ /*
+* relsubs_blocked[scanrelid - 1] is true if there is no EPQ tuple for
+* this target relation.
+*/
+ bool *relsubs_blocked;
Similarly, maybe say "no EPQ tuple for this target relation in a given
EvalPlanQual() invocation" here?
BTW, I didn't quite understand why EPQ involving resultRelations must
behave in this new way but not the EPQ during LockRows?
> What I'm thinking about doing to back-patch this is to replace
> one of the pointer fields in EPQState with a pointer to a
> subsidiary palloc'd structure, where we can put the new fields
> along with the cannibalized old one. We've done something
> similar before, and it seems a lot safer than having basically
> different logic in v16 than earlier branches.
+1.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: "variable not found in subplan target list"
Hi Alvaro, On Thu, May 4, 2023 at 19:44 Alvaro Herrera wrote: > On 2023-May-02, Alvaro Herrera wrote: > > > We have an open item about this, and I see no reason not to do it. I > > checked, and putting things back is just a matter of reverting > > 589bb816499e and ec386948948, cleaning up some trivial pgindent-induced > > conflicts, and bumping catversion once more. Would you like to do that > > yourself, or do you prefer that I do it? Ideally, we'd do it before > > beta1. > > I have pushed the revert now. Thanks for taking care of it. (Wouldn’t have been able to get to it till Monday myself.) > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: New committers: Nathan Bossart, Amit Langote, Masahiko Sawada
On Fri, Apr 21, 2023 at 17:52 Masahiko Sawada wrote: > On Fri, Apr 21, 2023 at 2:40 AM Tom Lane wrote: > > > > The Core Team would like to extend our congratulations to > > Nathan Bossart, Amit Langote, and Masahiko Sawada, who have > > accepted invitations to become our newest Postgres committers. > > > > Please join me in wishing them much success and few reverts. > > > > Thank you and everyone for the wishes! Congratulations to the other > new committers. +1, thank you core team for the opportunity. Thank you all for the wishes. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: on placeholder entries in view rule action query's range table
On Thu, Apr 6, 2023 at 3:33 Tom Lane wrote: > Amit Langote writes: > > While thinking about query view locking in context of [1], I realized > > that we have missed also fixing AcquirePlannerLocks() / > > ScanQueryForLocks() to consider that an RTE_SUBQUERY rte may belong to > > a view, which must be locked the same as RTE_RELATION entries. > > I think you're right about that, because AcquirePlannerLocks is supposed > to reacquire whatever locks parsing+rewriting would have gotten. > However, what's with this hunk? > > @@ -527,7 +527,7 @@ standard_planner(Query *parse, const char > *query_string, int cursorOptions, > result->partPruneInfos = glob->partPruneInfos; > result->rtable = glob->finalrtable; > result->permInfos = glob->finalrteperminfos; > - result->viewRelations = glob->viewRelations; > + result->viewRelations = NIL; > result->resultRelations = glob->resultRelations; > result->appendRelations = glob->appendRelations; > result->subplans = glob->subplans; Oops, I was working in the wrong local branch. Thanks for pushing. I agree that there’s no live bug as such right now, but still good to be consistent. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: generic plans and "initial" pruning
On Tue, Apr 4, 2023 at 10:29 PM Amit Langote wrote: > On Tue, Apr 4, 2023 at 6:41 AM Tom Lane wrote: > > A few concrete thoughts: > > > > * I understand that your plan now is to acquire locks on all the > > originally-named tables, then do permissions checks (which will > > involve only those tables), then dynamically lock just inheritance and > > partitioning child tables as we descend the plan tree. > > Actually, with the current implementation of the patch, *all* of the > relations mentioned in the plan tree would get locked during the > ExecInitNode() traversal of the plan tree (and of those in > plannedstmt->subplans), not just the inheritance child tables. > Locking of non-child tables done by the executor after this patch is > duplicative with AcquirePlannerLocks(), so that's something to be > improved. > > > That seems > > more or less okay to me, but it could be reflected better in the > > structure of the patch perhaps. > > > > * In particular I don't much like the "viewRelations" list, which > > seems like a wart; those ought to be handled more nearly the same way > > as other RTEs. (One concrete reason why is that this scheme is going > > to result in locking views in a different order than they were locked > > during original parsing, which perhaps could contribute to deadlocks.) > > Maybe we should store an integer list of which RTIs need to be locked > > in the early phase? Building that in the parser/rewriter would provide > > a solid guide to the original locking order, so we'd be trivially sure > > of duplicating that. (It might be close enough to follow the RT list > > order, which is basically what AcquireExecutorLocks does today, but > > this'd be more certain to do the right thing.) I'm less concerned > > about lock order for child tables because those are just going to > > follow the inheritance or partitioning structure. > > What you've described here sounds somewhat like what I had implemented > in the patch versions till v31, though it used a bitmapset named > minLockRelids that is initialized by setrefs.c. Your idea of > initializing a list before planning seems more appealing offhand than > the code I had added in setrefs.c to populate that minLockRelids > bitmapset, which would be bms_add_range(1, list_lenth(finalrtable)), > followed by bms_del_members(set-of-child-rel-rtis). > > I'll give your idea a try. After sleeping on this, I think we perhaps don't need to remember originally-named relations if only for the purpose of locking them for execution. That's because, for a reused (cached) plan, AcquirePlannerLocks() would have taken those locks anyway. AcquirePlannerLocks() doesn't lock inheritance children because they would be added to the range table by the planner, so they should be locked separately for execution, if needed. I thought taking the execution-time locks only when inside ExecInit[Merge]Append would work, but then we have cases where single-child Append/MergeAppend are stripped of the Append/MergeAppend nodes by setrefs.c. Maybe we need a place to remember such child relations, that is, only in the cases where Append/MergeAppend elision occurs, in something maybe esoteric-sounding like PlannedStmt.elidedAppendChildRels or something? Another set of child relations that are not covered by Append/MergeAppend child nodes is non-leaf partitions. I've proposed adding a List of Bitmapset field to Append/MergeAppend named 'allpartrelids' as part of this patchset (patch 0001) to track those for execution-time locking. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: on placeholder entries in view rule action query's range table
On Thu, Jan 12, 2023 at 10:06 AM Tom Lane wrote:
> Amit Langote writes:
> > On Mon, Jan 9, 2023 at 5:58 AM Tom Lane wrote:
> >> Conceivably we could make it work by allowing RTE_SUBQUERY RTEs to
> >> carry a relation OID and associated RTEPermissionInfo, so that when a
> >> view's RTE_RELATION RTE is transmuted to an RTE_SUBQUERY RTE it still
> >> carries the info needed to let us lock and permission-check the view.
> >> That might be a bridge too far from the ugliness perspective ...
> >> although certainly the business with OLD and/or NEW RTEs isn't very
> >> pretty either.
>
> > I had thought about that idea but was a bit scared of trying it,
> > because it does sound like something that might become a maintenance
> > burden in the future. Though I gave that a try today given that it
> > sounds like I may have your permission. ;-)
>
> Given the small number of places that need to be touched, I don't
> think it's a maintenance problem. I agree with your fear that you
> might have missed some, but I also searched and found no more.
While thinking about query view locking in context of [1], I realized
that we have missed also fixing AcquirePlannerLocks() /
ScanQueryForLocks() to consider that an RTE_SUBQUERY rte may belong to
a view, which must be locked the same as RTE_RELATION entries. Note
we did fix AcquireExecutorLocks() in 47bb9db75 as follows:
@@ -1769,7 +1769,8 @@ AcquireExecutorLocks(List *stmt_list, bool acquire)
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc2);
- if (rte->rtekind != RTE_RELATION)
+ if (!(rte->rtekind == RTE_RELATION ||
+ (rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid
Attached a patch to fix.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
[1] https://commitfest.postgresql.org/42/3478/
ScanQueryForLocks-lock-views.patch
Description: Binary data
Re: generic plans and "initial" pruning
On Tue, Apr 4, 2023 at 6:41 AM Tom Lane wrote: > Amit Langote writes: > > [ v38 patchset ] > > I spent a little bit of time looking through this, and concluded that > it's not something I will be wanting to push into v16 at this stage. > The patch doesn't seem very close to being committable on its own > terms, and even if it was now is not a great time in the dev cycle > to be making significant executor API changes. Too much risk of > having to thrash the API during beta, or even change it some more > in v17. I suggest that we push this forward to the next CF with the > hope of landing it early in v17. OK, thanks a lot for your feedback. > A few concrete thoughts: > > * I understand that your plan now is to acquire locks on all the > originally-named tables, then do permissions checks (which will > involve only those tables), then dynamically lock just inheritance and > partitioning child tables as we descend the plan tree. Actually, with the current implementation of the patch, *all* of the relations mentioned in the plan tree would get locked during the ExecInitNode() traversal of the plan tree (and of those in plannedstmt->subplans), not just the inheritance child tables. Locking of non-child tables done by the executor after this patch is duplicative with AcquirePlannerLocks(), so that's something to be improved. > That seems > more or less okay to me, but it could be reflected better in the > structure of the patch perhaps. > > * In particular I don't much like the "viewRelations" list, which > seems like a wart; those ought to be handled more nearly the same way > as other RTEs. (One concrete reason why is that this scheme is going > to result in locking views in a different order than they were locked > during original parsing, which perhaps could contribute to deadlocks.) > Maybe we should store an integer list of which RTIs need to be locked > in the early phase? Building that in the parser/rewriter would provide > a solid guide to the original locking order, so we'd be trivially sure > of duplicating that. (It might be close enough to follow the RT list > order, which is basically what AcquireExecutorLocks does today, but > this'd be more certain to do the right thing.) I'm less concerned > about lock order for child tables because those are just going to > follow the inheritance or partitioning structure. What you've described here sounds somewhat like what I had implemented in the patch versions till v31, though it used a bitmapset named minLockRelids that is initialized by setrefs.c. Your idea of initializing a list before planning seems more appealing offhand than the code I had added in setrefs.c to populate that minLockRelids bitmapset, which would be bms_add_range(1, list_lenth(finalrtable)), followed by bms_del_members(set-of-child-rel-rtis). I'll give your idea a try. > * I don't understand the need for changes like this: > > /* clean up tuple table */ > - ExecClearTuple(node->ps.ps_ResultTupleSlot); > + if (node->ps.ps_ResultTupleSlot) > + ExecClearTuple(node->ps.ps_ResultTupleSlot); > > ISTM that the process ought to involve taking a lock (if needed) > before we have built any execution state for a given plan node, > and if we find we have to fail, returning NULL instead of a > partially-valid planstate node. Otherwise, considerations of how > to handle partially-valid nodes are going to metastasize into all > sorts of places, almost certainly including EXPLAIN for instance. > I think we ought to be able to limit the damage to "parent nodes > might have NULL child links that you wouldn't have expected". > That wouldn't faze ExecEndNode at all, nor most other code. Hmm, yes, taking a lock before allocating any of the stuff to add into the planstate seems like it's much easier to reason about than the alternative I've implemented. > * More attention is needed to comments. For example, in a couple of > places in plancache.c you have removed function header comments > defining API details and not replaced them with any info about the new > details, despite the fact that those details are more complex than the > old. OK, yeah, maybe I've added a bunch of explanations in execMain.c that should perhaps have been in plancache.c. > > It seems I hadn't noted in the ExecEndNode()'s comment that all node > > types' recursive subroutines need to handle the change made by this > > patch that the corresponding ExecInitNode() subroutine may now return > > early without having initialized all state struct fields. > > Also noted in the documentation for CustomScan and ForeignScan that > > the Begin*Scan callback may not have been called at all, so the > > End*Scan should handle that gracefu
Re: SQL/JSON revisited
Hi Alvaro, On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera wrote: > On 2023-Mar-29, Alvaro Herrera wrote: > > In the meantime, here's the next two patches of the series: IS JSON and > > the "query" functions. I think this is as much as I can get done for > > this release, so the last two pieces of functionality would have to wait > > for 17. I still need to clean these up some more. These are not > > thoroughly tested either; 0001 compiles and passes regression tests, but > > I didn't verify 0003 other than there being no Git conflicts and bison > > doesn't complain. > > > > Also, notable here is that I realized that I need to backtrack on my > > change of the WITHOUT_LA: the original patch had it for TIME (in WITHOUT > > TIME ZONE), and I changed to be for UNIQUE. But now that I've done > > "JSON query functions" I realize that it needed to be the other way for > > the WITHOUT ARRAY WRAPPER clause too. So 0002 reverts that choice. > > So I pushed 0001 on Friday, and here are 0002 (which I intend to push > shortly, since it shouldn't be controversial) and the "JSON query > functions" patch as 0003. After looking at it some more, I think there > are some things that need to be addressed by one of the authors: > > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly. > I think we could make that stuff use something similar to > ConstraintAttributeSpec with an accompanying post-processing function. > That would reduce the number of ad-hoc hacks, which seem excessive. Do you mean the solution involving the JsonBehavior node? > - the changes in formatting.h have no explanation whatsoever. At the > very least, the new function should have a comment in the .c file. > (And why is it at end of file? I bet there's a better location) > > - some nasty hacks are being used in the ECPG grammar with no tests at > all. It's easy to add a few lines to the .pgc file I added in prior > commits. > > - Some functions in jsonfuncs.c have changed from throwing hard errors > into soft ones. I think this deserves more commentary. > > - func.sgml: The new functions are documented in a separate table for no > reason that I can see. Needs to be merged into one of the existing > tables. I didn't actually review the docs. I made the jsonfuncs.c changes to use soft error handling when needed, so I took a stab at that; attached a delta patch, which also fixes the problematic comments mentioned by Alexander in his comments 1 and 3. I'll need to spend some time to address other points. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v15-0002-delta.patch Description: Binary data
Re: "variable not found in subplan target list"
On Wed, Mar 29, 2023 at 3:39 AM Tom Lane wrote:
> Alvaro Herrera writes:
> > So I'm back home and found a couple more weird errors in the log:
>
> > ERROR: mismatching PartitionPruneInfo found at part_prune_index 0
> > DETALLE: plan node relids (b 1), pruneinfo relids (b 36)
>
> This one reproduces for me.
I've looked into this one and the attached patch fixes it for me.
Turns out set_plan_refs()'s idea of when the entries from
PlannerInfo.partPruneInfos are transferred into
PlannerGlobal.partPruneInfo was wrong.
Though, I wonder if we need to keep ec386948948 that introduced the
notion of part_prune_index around if the project that needed it [1]
has moved on to an entirely different approach altogether, one that
doesn't require hacking up the pruning code.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
[1] https://commitfest.postgresql.org/42/3478/
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 5cc8366af6..bd82960169 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -350,6 +350,29 @@ set_plan_references(PlannerInfo *root, Plan *plan)
palloc0(list_length(glob->subplans) * sizeof(bool));
}
+ /* Also fix up the information in PartitionPruneInfos. */
+ foreach(lc, root->partPruneInfos)
+ {
+ PartitionPruneInfo *pruneinfo = lfirst(lc);
+ ListCell *l;
+
+ pruneinfo->root_parent_relids =
+ offset_relid_set(pruneinfo->root_parent_relids, rtoffset);
+ foreach(l, pruneinfo->prune_infos)
+ {
+ List *prune_infos = lfirst(l);
+ ListCell *l2;
+
+ foreach(l2, prune_infos)
+ {
+PartitionedRelPruneInfo *pinfo = lfirst(l2);
+
+/* RT index of the table to which the pinfo belongs. */
+pinfo->rtindex += rtoffset;
+ }
+ }
+ }
+
/* Now fix the Plan tree */
result = set_plan_refs(root, plan, rtoffset);
@@ -378,31 +401,6 @@ set_plan_references(PlannerInfo *root, Plan *plan)
}
}
- /* Also fix up the information in PartitionPruneInfos. */
- foreach(lc, root->partPruneInfos)
- {
- PartitionPruneInfo *pruneinfo = lfirst(lc);
- ListCell *l;
-
- pruneinfo->root_parent_relids =
- offset_relid_set(pruneinfo->root_parent_relids, rtoffset);
- foreach(l, pruneinfo->prune_infos)
- {
- List *prune_infos = lfirst(l);
- ListCell *l2;
-
- foreach(l2, prune_infos)
- {
-PartitionedRelPruneInfo *pinfo = lfirst(l2);
-
-/* RT index of the table to which the pinfo belongs. */
-pinfo->rtindex += rtoffset;
- }
- }
-
- glob->partPruneInfos = lappend(glob->partPruneInfos, pruneinfo);
- }
-
return result;
}
@@ -1718,6 +1716,28 @@ set_customscan_references(PlannerInfo *root,
cscan->custom_relids = offset_relid_set(cscan->custom_relids, rtoffset);
}
+/*
+ * fix_part_prune_index
+ * Adds the PartitionPruneInfo present in root->partPruneIndex at given
+ * index into the list in PlannerGlobal and returns the index in the new
+ * list.
+ */
+static int
+fix_part_prune_index(int part_prune_index, PlannerInfo *root)
+{
+ PlannerGlobal *glob = root->glob;
+ PartitionPruneInfo *pruneinfo;
+
+ Assert(part_prune_index >= 0 &&
+ part_prune_index < list_length(root->partPruneInfos));
+ pruneinfo = list_nth_node(PartitionPruneInfo, root->partPruneInfos,
+ part_prune_index);
+
+ glob->partPruneInfos = lappend(glob->partPruneInfos, pruneinfo);
+
+ return list_length(glob->partPruneInfos) - 1;
+}
+
/*
* set_append_references
* Do set_plan_references processing on an Append
@@ -1771,11 +1791,11 @@ set_append_references(PlannerInfo *root,
aplan->apprelids = offset_relid_set(aplan->apprelids, rtoffset);
/*
- * PartitionPruneInfos will be added to a list in PlannerGlobal, so update
- * the index.
+ * Add PartitionPruneInfo, if any, to PlannerGlobal and update the index.
*/
if (aplan->part_prune_index >= 0)
- aplan->part_prune_index += list_length(root->glob->partPruneInfos);
+ aplan->part_prune_index =
+ fix_part_prune_index(aplan->part_prune_index, root);
/* We don't need to recurse to lefttree or righttree ... */
Assert(aplan->plan.lefttree == NULL);
@@ -1838,11 +1858,11 @@ set_mergeappend_references(PlannerInfo *root,
mplan->apprelids = offset_relid_set(mplan->apprelids, rtoffset);
/*
- * PartitionPruneInfos will be added to a list in PlannerGlobal, so update
- * the index.
+ * Add PartitionPruneInfo, if any, to PlannerGlobal and update the index.
*/
if (mplan->part_prune_index >= 0)
- mplan->part_prune_index += list_length(root->glob->partPruneInfos);
+ mplan->part_prune_index =
+ fix_part_prune_index(mplan->part_prune_index, root);
/* We don't need to recurse to lefttree or righttree ... */
Assert(mplan->plan.lefttree == NULL);
Re: SQL/JSON revisited
On Tue, Mar 28, 2023 at 6:18 AM Justin Pryzby wrote:
> I ran sqlsmith on this patch for a short while, and reduced one of its
> appalling queries to this:
>
> postgres=# SELECT jsonb_object_agg_unique_strict('', null::xid8);
> ERROR: unexpected jsonb type as object key
I think this may have to do with the following changes to
uniqueifyJsonbObject() that the patch makes:
@@ -1936,7 +1942,7 @@ lengthCompareJsonbPair(const void *a, const void
*b, void *binequal)
* Sort and unique-ify pairs in JsonbValue object
*/
static void
-uniqueifyJsonbObject(JsonbValue *object)
+uniqueifyJsonbObject(JsonbValue *object, bool unique_keys, bool skip_nulls)
{
boolhasNonUniq = false;
@@ -1946,15 +1952,32 @@ uniqueifyJsonbObject(JsonbValue *object)
qsort_arg(object->val.object.pairs, object->val.object.nPairs,
sizeof(JsonbPair),
lengthCompareJsonbPair, );
- if (hasNonUniq)
+ if (hasNonUniq && unique_keys)
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_JSON_OBJECT_KEY_VALUE),
+ errmsg("duplicate JSON object key value"));
+
+ if (hasNonUniq || skip_nulls)
{
- JsonbPair *ptr = object->val.object.pairs + 1,
- *res = object->val.object.pairs;
+ JsonbPair *ptr,
+ *res;
+
+ while (skip_nulls && object->val.object.nPairs > 0 &&
+ object->val.object.pairs->value.type == jbvNull)
+ {
+ /* If skip_nulls is true, remove leading items with null */
+ object->val.object.pairs++;
+ object->val.object.nPairs--;
+ }
+
+ ptr = object->val.object.pairs + 1;
+ res = object->val.object.pairs;
The code below the while loop does not take into account the
possibility that object->val.object.pairs would be pointing to garbage
when object->val.object.nPairs is 0.
Attached delta patch that applies on top of Alvaro's v12-0001 fixes
the case for me:
postgres=# SELECT jsonb_object_agg_unique_strict('', null::xid8);
jsonb_object_agg_unique_strict
{}
(1 row)
postgres=# SELECT jsonb_object_agg_unique_strict('1', null::xid8);
jsonb_object_agg_unique_strict
{}
(1 row)
SELECT jsonb_object_agg_unique_strict('1', '1'::xid8);
jsonb_object_agg_unique_strict
{"1": "1"}
(1 row)
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
v12-0001-delta-uniqueifyJsonbObject-bugfix.patch
Description: Binary data
Re: Eliminating SPI from RI triggers - take 2
Hi Greg, On Tue, Mar 21, 2023 at 3:54 AM Gregory Stark (as CFM) wrote: > On Mon, 17 Oct 2022 at 14:59, Robert Haas wrote: > > But I think the bigger problem for this patch set is that the > > design-level feedback from > > https://www.postgresql.org/message-id/CA%2BTgmoaiTNj4DgQy42OT9JmTTP1NWcMV%2Bke0i%3D%2Ba7%3DVgnzqGXw%40mail.gmail.com > > hasn't really been addressed, AFAICS. ri_LookupKeyInPkRelPlanIsValid > > is still trivial in v7, and that still seems wrong to me. And I still > > don't know how we're going to avoid changing the semantics in ways > > that are undesirable, or even knowing precisely what we did change. If > > we don't have answers to those questions, then I suspect that this > > patch set isn't going anywhere. > > Amit, do you plan to work on this patch for this commitfest (and > therefore this release?). And do you think it has a realistic chance > of being ready for commit this month? Unfortunately, I don't think so. > It looks to me like you have some good feedback and can progress and > are unlikely to finish this patch for this release. In which case > maybe we can move it forward to the next release? Yes, that's what I am thinking too at this point. I agree with Robert's point that changing the implementation from an SQL query plan to a hand-rolled C function is going to change the semantics in some known and perhaps many unknown ways. Until I have enumerated all those semantic changes, it's hard to judge whether the hand-rolled implementation is correct to begin with. I had started doing that a few months back but couldn't keep up due to some other work. An example I had found of a thing that would be broken by taking out the executor out of the equation, as the patch does, is the behavior of an update under READ COMMITTED isolation, whereby a PK tuple being checked for existence is concurrently updated and thus needs to rechecked whether it still satisfies the RI query's conditions. The executor has the EvalPlanQual() mechanism to do that, but while the hand-rolled implementation did refactor ExecLockRows() to allow doing the tuple-locking without a PlanState, it gave no consideration to handling rechecking under READ COMMITTED isolation. There may be other such things and I think I'd better look for them carefully in the next cycle than in the next couple of weeks for this release. My apologies that I didn't withdraw the patch sooner. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: A bug with ExecCheckPermissions
Hi Alvaro, On Thu, Mar 9, 2023 at 7:39 PM Alvaro Herrera wrote: > I didn't like very much this business of setting the perminfoindex > directly to '2' and '1'. It looks ugly with no explanation. What do > you think of creating the as we go along and set each index > correspondingly, as in the attached? Agree it looks cleaner and self-explanatory that way. Thanks. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: generic plans and "initial" pruning
On Wed, Feb 8, 2023 at 7:31 PM Amit Langote wrote: > On Tue, Feb 7, 2023 at 23:38 Andres Freund wrote: >> The tests seem to frequently hang on freebsd: >> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3478 > > Thanks for the heads up. I’ve noticed this one too, though couldn’t find the > testrun artifacts like I could get for some other failures (on other cirrus > machines). Has anyone else been a similar situation? I think I have figured out what might be going wrong on that cfbot animal after building with the same CPPFLAGS as that animal locally. I had forgotten to update _out/_readRangeTblEntry() to account for the patch's change that a view's RTE_SUBQUERY now also preserves relkind in addition to relid and rellockmode for the locking consideration. Also, I noticed that a multi-query Portal execution with rules was failing (thanks to a regression test added in a7d71c41db) because of the snapshot used for the 2nd query onward not being updated for command ID change under patched model of multi-query Portal execution. To wit, under the patched model, all queries in the multi-query Portal case undergo ExecutorStart() before any of it is run with ExecutorRun(). The patch hadn't changed things however to update the snapshot's command ID for the 2nd query onwards, which caused the aforementioned test case to fail. This new model does however mean that the 2nd query onwards must use PushCopiedSnapshot() given the current requirement of UpdateActiveSnapshotCommandId() that the snapshot passed to it must not be referenced anywhere else. The new model basically requires that each query's QueryDesc points to its own copy of the ActiveSnapshot. That may not be a thing in favor of the patched model though. For now, I haven't been able to come up with a better alternative. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v34-0001-Move-AcquireExecutorLocks-s-responsibility-into-.patch Description: Binary data
Re: SQL/JSON revisited
On Mon, Feb 27, 2023 at 4:45 PM Amit Langote wrote: > On Tue, Feb 21, 2023 at 2:25 AM Andres Freund wrote: > > Evaluating N expressions for a json table isn't a good approach, both memory > > and CPU efficiency wise. > > Are you referring to JsonTableInitOpaque() initializing all these > sub-expressions of JsonTableParent, especially colvalexprs, using N > *independent* ExprStates? That could perhaps be made to work by > making JsonTableParent be an expression recognized by execExpr.c, so > that a single ExprState can store the steps for all its > sub-expressions, much like JsonExpr is. I'll give that a try, though > I wonder if the semantics of making this work in a single > ExecEvalExpr() call will mismatch that of the current way, because > different sub-expressions are currently evaluated under different APIs > of TableFuncRoutine. I was looking at this and realized that using N ExprStates for various subsidiary expressions is not something specific to JSON_TABLE implementation. I mean we already have bunch of ExprStates being created in ExecInitTableFuncScan(): scanstate->ns_uris = ExecInitExprList(tf->ns_uris, (PlanState *) scanstate); scanstate->docexpr = ExecInitExpr((Expr *) tf->docexpr, (PlanState *) scanstate); scanstate->rowexpr = ExecInitExpr((Expr *) tf->rowexpr, (PlanState *) scanstate); scanstate->colexprs = ExecInitExprList(tf->colexprs, (PlanState *) scanstate); scanstate->coldefexprs = ExecInitExprList(tf->coldefexprs, (PlanState *) scanstate); Or maybe you're worried about jsonpath_exec.c using so many ExprStates *privately* to put into TableFuncScanState.opaque? -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Tue, Feb 21, 2023 at 12:40 AM Tom Lane wrote: > Alvaro Herrera writes: > > On 2023-Feb-20, Amit Langote wrote: > >> One more thing we could try is come up with a postgres_fdw test case, > >> because it uses the RelOptInfo.userid value for remote-costs-based > >> path size estimation. But adding a test case to contrib module's > >> suite test a core planner change might seem strange, ;-). > > > Maybe. Perhaps adding it in a separate file there is okay? > > There is plenty of stuff in contrib module tests that is really > there to test core-code behavior. (You could indeed argue that > *all* of contrib is there for that purpose.) If it's not > convenient to test something without an extension, just do it > and don't sweat about that. OK. Attached adds a test case to postgres_fdw's suite. You can see that it fails without a316a3bc. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v1-0001-postgres_fdw-test-userid-propagation-to-rels-unde.patch Description: Binary data
Re: SQL/JSON revisited
On Tue, Feb 21, 2023 at 12:09 PM Amit Langote wrote: > On Mon, Feb 20, 2023 at 11:41 PM Erik Rijkers wrote: > > Op 20-02-2023 om 08:35 schreef Amit Langote: > > > Rebased again over queryjumble overhaul. > > But the following statement is a problem. It does not crash but it goes > > off, half-freezing the machine, and only comes back after fanatic > > Ctrl-C'ing. > > > > select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object > > on error); > > > > Can you have a look? > > Thanks for the test case. It caused ExecInterpExpr() to enter an > infinite loop, which I've fixed in the attached updated version. I've > also merged Elena's documentation changes; I can see that > is more correct. Oops, I hadn't actually done the latter. Will do when posting the next version. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Fri, Feb 17, 2023 at 9:02 PM Alvaro Herrera wrote: > On 2022-Dec-11, Amit Langote wrote: > > While staring at the build_simple_rel() bit mentioned above, I > > realized that this code fails to set userid correctly in the > > inheritance parent rels that are child relations of subquery parent > > relations, such as UNION ALL subqueries. In that case, instead of > > copying the userid (= 0) of the parent rel, the child should look up > > its own RTEPermissionInfo, which should be there, and use the > > checkAsUser value from there. I've attached 0002 to fix this hole. I > > am not sure whether there's a way to add a test case for this in the > > core suite. > > I gave this a look and I thought it was clearer to have the new > condition depend on rel->reloptkind instead parent or no. Thanks for looking into this again. I agree the condition with reloptkind might be better. > I tried a few things for a new test case, but I was unable to find > anything useful. Maybe an intermediate view, I thought; no dice. > Maybe one with a security barrier would do? Anyway, for now I just kept > what you added in v2. Hmm, I'm fine with leaving the test case out if it doesn't really test the code we're changing, as you also pointed out? One more thing we could try is come up with a postgres_fdw test case, because it uses the RelOptInfo.userid value for remote-costs-based path size estimation. But adding a test case to contrib module's suite test a core planner change might seem strange, ;-). Attaching v4 without the test case. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v4-0001-Correctly-set-userid-of-subquery-rel-s-child-rels.patch Description: Binary data
Re: ExecRTCheckPerms() and many prunable partitions (sqlsmith)
On Mon, Feb 13, 2023 at 22:31 Tom Lane wrote: > Amit Langote writes: > > On Mon, Feb 13, 2023 at 5:07 Justin Pryzby wrote: > >> That seems to add various elog()s which are hit frequently by sqlsmith: > > > Thanks for the report. I’ll take a look once I’m back at a computer in a > > few days. > > Looks like we already have a diagnosis and fix [1]. I'll get that > pushed. > > regards, tom lane > > [1] > https://www.postgresql.org/message-id/CAHewXNnnNySD_YcKNuFpQDV2gxWA7_YLWqHmYVcyoOYxn8kY2A%40mail.gmail.com Oh, thanks a lot. > > <https://www.postgresql.org/message-id/CAHewXNnnNySD_YcKNuFpQDV2gxWA7_YLWqHmYVcyoOYxn8kY2A%40mail.gmail.com> -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: ExecRTCheckPerms() and many prunable partitions (sqlsmith)
On Mon, Feb 13, 2023 at 5:07 Justin Pryzby wrote: > On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote: > > 0002 contains changes that has to do with changing how we access > > checkAsUser in some foreign table planning/execution code sites. > > Thought it might be better to describe it separately too. > > This was committed as 599b33b94: > Stop accessing checkAsUser via RTE in some cases > > That seems to add various elog()s which are hit frequently by sqlsmith: > > postgres=# select from > (select transaction > from pg_prepared_xacts > right join pg_available_extensions > on false limit 0) where false; > ERROR: permission info at index 2 (with relid=1262) does not match > provided RTE (with relid=12081) > > postgres=# select from (select confl_tablespace > from pg_stat_database_conflicts > where datname <> (select 'af') > limit 1) where false; > ERROR: invalid perminfoindex 1 in RTE with relid 12271 Thanks for the report. I’ll take a look once I’m back at a computer in a few days. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: A bug with ExecCheckPermissions
Hi, On Thu, Feb 9, 2023 at 14:44 Sergey Shinderuk wrote: > On 08.02.2023 21:23, Alvaro Herrera wrote: > > On 2023-Feb-08, Amit Langote wrote: > > > >> On Wed, Feb 8, 2023 at 16:19 Alvaro Herrera > wrote: > > > >>> I think we should also patch ExecCheckPermissions to use forboth(), > >>> scanning the RTEs as it goes over the perminfos, and make sure that the > >>> entries are consistent. > >> > >> Hmm, we can’t use forboth here, because not all RTEs have the > corresponding > >> RTEPermissionInfo, inheritance children RTEs, for example. > > > > Doh, of course. > > > >> Also, it doesn’t make much sense to reinstate the original loop over > >> range table and fetch the RTEPermissionInfo for the RTEs with non-0 > >> perminfoindex, because the main goal of the patch was to make > >> ExecCheckPermissions() independent of range table length. > > > > Yeah, I'm thinking in a mechanism that would allow us to detect bugs in > > development builds — no need to have it run in production builds. > > However, I can't see any useful way to implement it. > > > > > Maybe something like the attached would do? Thanks for the patch. Something like this makes sense to me. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: A bug with ExecCheckPermissions
On Wed, Feb 8, 2023 at 16:19 Alvaro Herrera wrote: > On 2023-Feb-08, o.tselebrovs...@postgrespro.ru wrote: > > > But if you debug function ExecCheckPermissions and look into what is > passed > > to function (contents of rangeTable and rteperminfos to be exact), > > you'll see some strange behaviour: > > > Both of RangeTableEntries have a perminfoindex of 0 and simultaneously > have > > a RTEPERMISSIONINFO entry for them! > > Ouch. Yeah, that's not great. As you say, it doesn't really affect > anything, and we know full well that these RTEs are ad-hoc > manufactured. But as we claim that we still pass the RTEs for the > benefit of hooks, then we should at least make them match. +1. We don’t have anything in this (core) code path that would try to use perminfoindex for these RTEs, but there might well be in the future. I think we should also patch ExecCheckPermissions to use forboth(), > scanning the RTEs as it goes over the perminfos, and make sure that the > entries are consistent. Hmm, we can’t use forboth here, because not all RTEs have the corresponding RTEPermissionInfo, inheritance children RTEs, for example. Also, it doesn’t make much sense to reinstate the original loop over range table and fetch the RTEPermissionInfo for the RTEs with non-0 perminfoindex, because the main goal of the patch was to make ExecCheckPermissions() independent of range table length. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: generic plans and "initial" pruning
On Tue, Feb 7, 2023 at 23:38 Andres Freund wrote: > Hi, > > On 2023-02-03 22:01:09 +0900, Amit Langote wrote: > > I've added a test case under src/modules/delay_execution by adding a > > new ExecutorStart_hook that works similarly as > > delay_execution_planner(). The test works by allowing a concurrent > > session to drop an object being referenced in a cached plan being > > initialized while the ExecutorStart_hook waits to get an advisory > > lock. The concurrent drop of the referenced object is detected during > > ExecInitNode() and thus triggers replanning of the cached plan. > > > > I also fixed a bug in the ExplainExecuteQuery() while testing and some > comments. > > The tests seem to frequently hang on freebsd: > > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3478 Thanks for the heads up. I’ve noticed this one too, though couldn’t find the testrun artifacts like I could get for some other failures (on other cirrus machines). Has anyone else been a similar situation? > > <https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3478> -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: generic plans and "initial" pruning
On Thu, Feb 2, 2023 at 11:49 PM Amit Langote wrote: > On Fri, Jan 27, 2023 at 4:01 PM Amit Langote wrote: > > I didn't actually go with calling the plancache on every lock taken on > > a relation, that is, in ExecGetRangeTableRelation(). One thing about > > doing it that way that I didn't quite like (or didn't see a clean > > enough way to code) is the need to complicate the ExecInitNode() > > traversal for handling the abrupt suspension of the ongoing setup of > > the PlanState tree. > > OK, I gave this one more try and attached is what I came up with. > > This adds a ExecPlanStillValid(), which is called right after anything > that may in turn call ExecGetRangeTableRelation() which has been > taught to lock a relation if EXEC_FLAG_GET_LOCKS has been passed in > EState.es_top_eflags. That includes all ExecInitNode() calls, and a > few other functions that call ExecGetRangeTableRelation() directly, > such as ExecOpenScanRelation(). If ExecPlanStillValid() returns > false, that is, if EState.es_cachedplan is found to have been > invalidated after a lock being taken by ExecGetRangeTableRelation(), > whatever funcion called it must return immediately and so must its > caller and so on. ExecEndPlan() seems to be able to clean up after a > partially finished attempt of initializing a PlanState tree in this > way. Maybe my preliminary testing didn't catch cases where pointers > to resources that are normally put into the nodes of a PlanState tree > are now left dangling, because a partially built PlanState tree is not > accessible to ExecEndPlan; QueryDesc.planstate would remain NULL in > such cases. Maybe there's only es_tupleTable and es_relations that > needs to be explicitly released and the rest is taken care of by > resetting the ExecutorState context. In the attached updated patch, I've made the functions that check ExecPlanStillValid() to return NULL (if returning something) instead of returning partially initialized structs. Those partially initialized structs were not being subsequently looked at anyway. > On testing, I'm afraid we're going to need something like > src/test/modules/delay_execution to test that concurrent changes to > relation(s) in PlannedStmt.relationOids that occur somewhere between > RevalidateCachedQuery() and InitPlan() result in the latter to be > aborted and that it is handled correctly. It seems like it is only > the locking of partitions (that are not present in an unplanned Query > and thus not protected by AcquirePlannerLocks()) that can trigger > replanning of a CachedPlan, so any tests we write should involve > partitions. Should this try to test as many plan shapes as possible > though given the uncertainty around ExecEndPlan() robustness or should > manual auditing suffice to be sure that nothing's broken? I've added a test case under src/modules/delay_execution by adding a new ExecutorStart_hook that works similarly as delay_execution_planner(). The test works by allowing a concurrent session to drop an object being referenced in a cached plan being initialized while the ExecutorStart_hook waits to get an advisory lock. The concurrent drop of the referenced object is detected during ExecInitNode() and thus triggers replanning of the cached plan. I also fixed a bug in the ExplainExecuteQuery() while testing and some comments. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v33-0001-Move-AcquireExecutorLocks-s-responsibility-into-.patch Description: Binary data
Re: generic plans and "initial" pruning
On Fri, Jan 27, 2023 at 4:01 PM Amit Langote wrote: > On Fri, Jan 20, 2023 at 12:52 PM Amit Langote wrote: > > Alright, I'll try to get something out early next week. Thanks for > > all the pointers. > > Sorry for the delay. Attached is what I've come up with so far. > > I didn't actually go with calling the plancache on every lock taken on > a relation, that is, in ExecGetRangeTableRelation(). One thing about > doing it that way that I didn't quite like (or didn't see a clean > enough way to code) is the need to complicate the ExecInitNode() > traversal for handling the abrupt suspension of the ongoing setup of > the PlanState tree. OK, I gave this one more try and attached is what I came up with. This adds a ExecPlanStillValid(), which is called right after anything that may in turn call ExecGetRangeTableRelation() which has been taught to lock a relation if EXEC_FLAG_GET_LOCKS has been passed in EState.es_top_eflags. That includes all ExecInitNode() calls, and a few other functions that call ExecGetRangeTableRelation() directly, such as ExecOpenScanRelation(). If ExecPlanStillValid() returns false, that is, if EState.es_cachedplan is found to have been invalidated after a lock being taken by ExecGetRangeTableRelation(), whatever funcion called it must return immediately and so must its caller and so on. ExecEndPlan() seems to be able to clean up after a partially finished attempt of initializing a PlanState tree in this way. Maybe my preliminary testing didn't catch cases where pointers to resources that are normally put into the nodes of a PlanState tree are now left dangling, because a partially built PlanState tree is not accessible to ExecEndPlan; QueryDesc.planstate would remain NULL in such cases. Maybe there's only es_tupleTable and es_relations that needs to be explicitly released and the rest is taken care of by resetting the ExecutorState context. On testing, I'm afraid we're going to need something like src/test/modules/delay_execution to test that concurrent changes to relation(s) in PlannedStmt.relationOids that occur somewhere between RevalidateCachedQuery() and InitPlan() result in the latter to be aborted and that it is handled correctly. It seems like it is only the locking of partitions (that are not present in an unplanned Query and thus not protected by AcquirePlannerLocks()) that can trigger replanning of a CachedPlan, so any tests we write should involve partitions. Should this try to test as many plan shapes as possible though given the uncertainty around ExecEndPlan() robustness or should manual auditing suffice to be sure that nothing's broken? On possibly needing to move permission checking to occur *after* taking locks, I realized that we don't really need to, because no relation that needs its permissions should be unlocked by the time we get to ExecCheckPermissions(); note we only check permissions of tables that are present in the original parse tree and RevalidateCachedQuery() should have locked those. I found a couple of exceptions to that invariant in that views sometimes appear not to be in the set of relations that RevalidateCachedQuery() locks. So, I invented PlannedStmt.viewRelations, a list of RT indexes of view RTEs that is populated in setrefs.c. ExecLockViewRelations() called before ExecCheckPermissions() locks those. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v32-0001-Move-AcquireExecutorLocks-s-responsibility-into-.patch Description: Binary data
Re: Speed up transaction completion faster after many relations are accessed in a transaction
Hi David,
On Tue, Jan 24, 2023 at 12:58 PM David Rowley wrote:
> On Fri, 20 Jan 2023 at 00:26, vignesh C wrote:
> > CFBot shows some compilation errors as in [1], please post an updated
> > version for the same:
>
> I've attached a rebased patch.
Thanks for the new patch.
Maybe you're planning to do it once this patch is post the PoC phase
(isn't it?), but it would be helpful to have commentary on all the new
dlist fields.
Especially, I think the following warrants a bit more explanation than other:
- /* We can remember up to MAX_RESOWNER_LOCKS references to local locks. */
- int nlocks; /* number of owned locks */
- LOCALLOCK *locks[MAX_RESOWNER_LOCKS]; /* list of owned locks */
+ dlist_head locks; /* dlist of owned locks */
This seems to be replacing what is a cache with an upper limit on the
number of cacheds locks with something that has no limit on how many
per-owner locks are remembered. I wonder whether we'd be doing
additional work in some cases with the new no-limit implementation
that wasn't being done before (where the owner's locks array is
overflowed) or maybe not much because the new implementation of
ResourceOwner{Remember|Forget}Lock() is simple push/delete of a dlist
node from the owner's dlist?
The following comment is now obsolete:
/*
* LockReassignCurrentOwner
* Reassign all locks belonging to CurrentResourceOwner to belong
* to its parent resource owner.
*
* If the caller knows what those locks are, it can pass them as an array.
* That speeds up the call significantly, when a lot of locks are held
* (e.g pg_dump with a large schema). Otherwise, pass NULL for locallocks,
* and we'll traverse through our hash table to find them.
*/
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: wrong Append/MergeAppend elision?
On Fri, Jan 27, 2023 at 5:43 AM Tom Lane wrote: > David Rowley writes: > > On Fri, 27 Jan 2023 at 01:30, Amit Langote wrote: > >> It seems that the planner currently elides an Append/MergeAppend that > >> has run-time pruning info (part_prune_index) set, but which I think is > >> a bug. > > > There is still the trade-off of having to pull tuples through the > > Append node for when run-time pruning is unable to prune the last > > partition. So your proposal to leave the Append alone when there's > > run-time pruning info is certainly not a no-brainer. > > Yeah. Amit's proposal amounts to optimizing for the case that all > partitions get pruned, which does not seem to me to be the way > to bet. I'm inclined to think it's fine as-is. Fair enough. I thought for a second that maybe it was simply an oversight but David confirms otherwise. This was interacting badly with the other patch I'm working on and I just figured out the problem was with that other patch. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
wrong Append/MergeAppend elision?
Hi, It seems that the planner currently elides an Append/MergeAppend that has run-time pruning info (part_prune_index) set, but which I think is a bug. Here's an example: create table p (a int) partition by list (a); create table p1 partition of p for values in (1); set plan_cache_mode to force_generic_plan ; prepare q as select * from p where a = $1; explain execute q (0); QUERY PLAN -- Seq Scan on p1 p (cost=0.00..41.88 rows=13 width=4) Filter: (a = $1) (2 rows) Because the Append is elided in this case, run-time pruning doesn't kick in to prune p1, even though PartitionPruneInfo to do so has been generated. Attached find a patch to fix that. There are some expected output diffs in partition_prune suite, though they all look sane to me. Thoughts? -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v1-0001-Don-t-elide-Append-MergeAppend-if-run-time-prunin.patch Description: Binary data
Re: generic plans and "initial" pruning
On Fri, Jan 20, 2023 at 12:58 PM Tom Lane wrote: > Amit Langote writes: > > On Fri, Jan 20, 2023 at 12:31 PM Tom Lane wrote: > >> It might be possible to incorporate this pointer into PlannedStmt > >> instead of passing it separately. > > > Yeah, that would be less churn. Though, I wonder if you still hold > > that PlannedStmt should not be scribbled upon outside the planner as > > you said upthread [1]? > > Well, the whole point of that rule is that the executor can't modify > a plancache entry. If the plancache itself sets a field in such an > entry, that doesn't seem problematic from here. > > But there's other possibilities if that bothers you; QueryDesc > could hold the field, for example. Also, I bet we'd want to copy > it into EState for the main initialization recursion. QueryDesc sounds good to me, and yes, also a copy in EState in any case. So I started looking at the call sites of CreateQueryDesc() and stopped to look at ExecParallelGetQueryDesc(). AFAICS, we wouldn't need to pass the CachedPlan to a parallel worker's rerun of InitPlan(), because 1) it doesn't make sense to call the plancache in a parallel worker, 2) the leader should already have taken all the locks in necessary for executing a given plan subnode that it intends to pass to a worker in ExecInitGather(). Does that make sense? -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: generic plans and "initial" pruning
On Fri, Jan 20, 2023 at 12:31 PM Tom Lane wrote: > Amit Langote writes: > > On Fri, Jan 20, 2023 at 4:39 AM Tom Lane wrote: > >> I had what felt like an epiphany: the whole problem arises because the > >> system is wrongly factored. We should get rid of AcquireExecutorLocks > >> altogether, allowing the plancache to hand back a generic plan that > >> it's not certain of the validity of, and instead integrate the > >> responsibility for acquiring locks into executor startup. > > > Interesting. The current implementation relies on > > PlanCacheRelCallback() marking a generic CachedPlan as invalid, so > > perhaps there will have to be some sharing of state between the > > plancache and the executor for this to work? > > Yeah. Thinking a little harder, I think this would have to involve > passing a CachedPlan pointer to the executor, and what the executor > would do after acquiring each lock is to ask the plancache "hey, do > you still think this CachedPlan entry is valid?". In the case where > there's a problem, the AcceptInvalidationMessages call involved in > lock acquisition would lead to a cache inval that clears the validity > flag on the CachedPlan entry, and this would provide an inexpensive > way to check if that happened. OK, thanks, this is useful. > It might be possible to incorporate this pointer into PlannedStmt > instead of passing it separately. Yeah, that would be less churn. Though, I wonder if you still hold that PlannedStmt should not be scribbled upon outside the planner as you said upthread [1]? > >> * In a successfully built execution state tree, there will simply > >> not be any nodes corresponding to pruned-away, never-locked subplans. > > > I think this is true with the patch as proposed too, but I was still a > > bit worried about what an ExecutorStart_hook may be doing with an > > uninitialized plan tree. Maybe we're mandating that the hook must > > call standard_ExecutorStart() and only work with the finished > > PlanState tree? > > It would certainly be incumbent on any such hook to not touch > not-yet-locked parts of the plan tree. I'm not particularly concerned > about that sort of requirements change, because we'd be breaking APIs > all through this area in any case. OK. Perhaps something that should be documented around ExecutorStart(). > >> * In some cases (views, at least) we need to acquire lock on relations > >> that aren't directly reflected anywhere in the plan tree. So there'd > >> have to be a separate mechanism for getting those locks and rechecking > >> validity afterward. A list of relevant relation OIDs might be enough > >> for that. > > > Hmm, a list of only the OIDs wouldn't preserve the lock mode, > > Good point. I wonder if we could integrate this with the > RTEPermissionInfo data structure? You mean adding a rellockmode field to RTEPermissionInfo? > > Would you like me to hack up a PoC or are you already on that? > > I'm not planning to work on this myself, I was hoping you would. Alright, I'll try to get something out early next week. Thanks for all the pointers. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://www.postgresql.org/message-id/922566.1648784745%40sss.pgh.pa.us
Re: generic plans and "initial" pruning
a fair > amount of executor startup overhead before discovering we have > to throw all that state away. I think that's clearly the right > way to bet, but perhaps somebody else has a different view. Not sure if you'd like, because it would still keep the PartitionPruneResult business, but this will be less of a problem if we do the initial pruning at the beginning of InitPlan(), followed by locking, before doing anything else. We would have initialized the QueryDesc and the EState, but only minimally. That also keeps the PartitionPruneResult business local to the executor. Would you like me to hack up a PoC or are you already on that? -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://www.postgresql.org/message-id/CA%2BHiwqG7ZruBmmih3wPsBZ4s0H2EhywrnXEduckY5Hr3fWzPWA%40mail.gmail.com
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Tue, Jan 17, 2023 at 7:33 PM Alvaro Herrera wrote: > On 2022-Dec-12, Amit Langote wrote: > > On Sun, Dec 11, 2022 at 6:25 PM Amit Langote > > wrote: > > > I've attached 0001 to remove those extraneous code blocks and add a > > > comment mentioning that userid need not be recomputed. > > > > > > While staring at the build_simple_rel() bit mentioned above, I > > > realized that this code fails to set userid correctly in the > > > inheritance parent rels that are child relations of subquery parent > > > relations, such as UNION ALL subqueries. In that case, instead of > > > copying the userid (= 0) of the parent rel, the child should look up > > > its own RTEPermissionInfo, which should be there, and use the > > > checkAsUser value from there. I've attached 0002 to fix this hole. I > > > am not sure whether there's a way to add a test case for this in the > > > core suite. > > > > Ah, I realized we could just expand the test added by 553d2ec27 with a > > wrapper view (to test checkAsUser functionality) and a UNION ALL query > > over the view (to test this change). > > Hmm, but if I run this test without the code change in 0002, the test > also passes (to wit: the plan still has two hash joins), so I understand > that either you're testing something that's not changed by the patch, > or the test case itself isn't really what you wanted. Yeah, the test case is bogus. :-(. It seems that, with the test as written, it's not the partitioned table referenced in the view's query that becomes a child of the UNION ALL parent subquery, but the subquery itself. The bug being fixed in 0002 doesn't affect the planning of this query at all, because child subquery is planned independently of the main query involving UNION ALL because of it being unable to be pushed up into the latter. We want the partitioned table referenced in the child subquery to become a child of the UNION ALL parent subquery for the bug to be relevant. I tried rewriting the test such that the view's subquery does get pulled up such that the partitioned table becomes a child of the UNION ALL subquery. By attaching a debugger, I do see the bug affecting the planning of this query, but still not in a way that changes the plan. I will keep trying but in the meantime I'm attaching 0001 to show the rewritten query and the plan. > As for 0001, it seems simpler to me to put the 'userid' variable in the > same scope as 'onerel', and then compute it just once and don't bother > with it at all afterwards, as in the attached. That sounds better. Attached as 0002. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v3-0001-Add-test-case-to-test-a-bug-of-build_simple_rel.patch Description: Binary data v3-0002-Remove-some-dead-code-in-selfuncs.c.patch Description: Binary data v3-0003-Correctly-set-userid-of-subquery-rel-s-child-rels.patch Description: Binary data
Re: on placeholder entries in view rule action query's range table
On Thu, Jan 12, 2023 at 12:45 PM Tom Lane wrote: > Amit Langote writes: > > On Thu, Jan 12, 2023 at 10:06 AM Tom Lane wrote: > >> I've pushed this with some cleanup --- aside from fixing > >> outfuncs/readfuncs, I did some more work on the comments, which > >> I think you were too sloppy about. > > > Thanks a lot for the fixes. > > It looks like we're not out of the woods on this: the buildfarm > members that run cross-version-upgrade tests are all unhappy. > Most of them are not reporting any useful details, but I suspect > that they are barfing because dumps from the old server include > table-qualified variable names in some CREATE VIEW commands while > dumps from HEAD omit the qualifications. I don't see any > mechanism in TestUpgradeXversion.pm that could deal with that > conveniently, and in any case we'd have to roll out a client > script update to the affected animals. I fear we may have to > revert this pending development of better TestUpgradeXversion.pm > support. Ah, OK, no problem. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: ATTACH PARTITION seems to ignore column generation status
On Thu, Jan 12, 2023 at 5:58 AM Tom Lane wrote: > Amit Langote writes: > > I've updated your disallow-generated-child-columns-2.patch to do this, > > and have also merged the delta post that I had attached with my last > > email, whose contents you sound to agree with. > > Pushed with some further work to improve the handling of multiple- > inheritance cases. We still need to insist that all or none of the > parent columns are generated, but we don't have to require their > generation expressions to be alike: that can be resolved by letting > the child table override the expression, much as we've long done for > plain default expressions. (This did need some work in pg_dump > after all.) I'm pretty happy with where this turned out. Thanks, that all looks more consistent now indeed. I noticed a typo in the doc additions, which I've attached a fix for. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com 8bf6ec3ba3-doc-typo.patch Description: Binary data
Re: on placeholder entries in view rule action query's range table
On Thu, Jan 12, 2023 at 10:06 AM Tom Lane wrote:
> Amit Langote writes:
> > On Mon, Jan 9, 2023 at 5:58 AM Tom Lane wrote:
> >> Conceivably we could make it work by allowing RTE_SUBQUERY RTEs to
> >> carry a relation OID and associated RTEPermissionInfo, so that when a
> >> view's RTE_RELATION RTE is transmuted to an RTE_SUBQUERY RTE it still
> >> carries the info needed to let us lock and permission-check the view.
> >> That might be a bridge too far from the ugliness perspective ...
> >> although certainly the business with OLD and/or NEW RTEs isn't very
> >> pretty either.
>
> > I had thought about that idea but was a bit scared of trying it,
> > because it does sound like something that might become a maintenance
> > burden in the future. Though I gave that a try today given that it
> > sounds like I may have your permission. ;-)
>
> Given the small number of places that need to be touched, I don't
> think it's a maintenance problem. I agree with your fear that you
> might have missed some, but I also searched and found no more.
OK, thanks.
> > I was
> > surprised that nothing failed with a -DWRITE_READ_PARSE_PLAN_TREES
> > build, because of the way RTEs are written and read -- relid,
> > rellockmode are not written/read for RTE_SUBQUERY RTEs.
>
> I think that's mostly accidental, stemming from the facts that:
> (1) Stored rules wouldn't have these fields populated yet anyway.
> (2) The regression tests haven't got any good way to check that a
> needed lock was actually acquired. It looks to me like with the
> patch as you have it, when a plan tree is copied into the plan
> cache the view relid is lost (if pg_plan_query stripped it thanks
> to -DWRITE_READ_PARSE_PLAN_TREES) and thus we won't re-acquire the
> view lock in AcquireExecutorLocks during later plan uses. But
> that would have no useful effect unless it forced a re-plan due to
> a concurrent view replacement, which is a scenario I'm pretty sure
> we don't actually exercise in the tests.
Ah, does it make sense to have isolation tests cover this?
> (3) The executor doesn't look at these fields after startup, so
> failure to transmit them to parallel workers doesn't hurt.
>
> In any case, it would clearly be very foolish not to fix
> outfuncs/readfuncs to preserve all the fields we're using.
>
> I've pushed this with some cleanup --- aside from fixing
> outfuncs/readfuncs, I did some more work on the comments, which
> I think you were too sloppy about.
Thanks a lot for the fixes.
> Sadly, the original nested-views test case still has O(N^2)
> planning time :-(. I dug into that harder and now see where
> the problem really lies. The rewriter recursively replaces
> the view RTE_RELATION RTEs with RTE_SUBQUERY all the way down,
> which takes it only linear time. However, then we have a deep
> nest of RTE_SUBQUERYs, and the initial copyObject in
> pull_up_simple_subquery repeatedly copies everything below the
> current pullup recursion level, so that it's still O(N^2)
> even though the final rtable will have only N entries.
That makes sense.
> I'm afraid to remove the copyObject step, because that would
> cause problems in the cases where we try to perform pullup
> and have to abandon it later. (Maybe we could get rid of
> all such cases, but I'm not sanguine about that succeeding.)
> I'm tempted to try to fix it by taking view replacement out
> of the rewriter altogether and making prepjointree.c handle
> it during the same recursive scan that does subquery pullup,
> so that we aren't applying copyObject to already-expanded
> RTE_SUBQUERY nests. However, that's more work than I care to
> put into the problem right now.
OK, I will try to give your idea a shot sometime later.
BTW, I noticed that we could perhaps remove the following in the
fireRIRrules()'s loop that calls ApplyRetrieveRule(), because we no
longer put any unreferenced OLD/NEW RTEs in the view queries.
/*
* If the table is not referenced in the query, then we ignore it.
* This prevents infinite expansion loop due to new rtable entries
* inserted by expansion of a rule. A table is referenced if it is
* part of the join set (a source table), or is referenced by any Var
* nodes, or is the result table.
*/
if (rt_index != parsetree->resultRelation &&
!rangeTableEntry_used((Node *) parsetree, rt_index, 0))
continue;
Commenting this out doesn't break make check.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Re: ATTACH PARTITION seems to ignore column generation status
On Wed, Jan 11, 2023 at 7:13 AM Tom Lane wrote:
> I wrote:
> > Amit Langote writes:
> >> Thanks for the patch. It looks good, though I guess you said that we
> >> should also change the error message that CREATE TABLE ... PARTITION
> >> OF emits to match the other cases while we're here. I've attached a
> >> delta patch.
>
> > Thanks. I hadn't touched that issue because I wasn't entirely sure
> > which error message(s) you were unhappy with. These changes look
> > fine offhand.
>
> So, after playing with that a bit ... removing the block in
> parse_utilcmd.c allows you to do
>
> regression=# CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3
> bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1);
> CREATE TABLE
> regression=# CREATE TABLE gtest_child PARTITION OF gtest_parent (
> regression(#f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 3) STORED
> regression(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> CREATE TABLE
> regression=# \d gtest_child
> Table "public.gtest_child"
> Column | Type | Collation | Nullable | Default
> ++---+--+-
> f1 | date | | not null |
> f2 | bigint | | |
> f3 | bigint | | | generated always as (f2 * 3) stored
> Partition of: gtest_parent FOR VALUES FROM ('2016-07-01') TO ('2016-08-01')
>
> regression=# insert into gtest_parent values('2016-07-01', 42);
> INSERT 0 1
> regression=# table gtest_parent;
> f1 | f2 | f3
> ++-
> 2016-07-01 | 42 | 126
> (1 row)
>
> That is, you can make a partition with a different generated expression
> than the parent has. Do we really want to allow that? I think it works
> as far as the backend is concerned, but it breaks pg_dump, which tries
> to dump this state of affairs as
>
> CREATE TABLE public.gtest_parent (
> f1 date NOT NULL,
> f2 bigint,
> f3 bigint GENERATED ALWAYS AS ((f2 * 2)) STORED
> )
> PARTITION BY RANGE (f1);
>
> CREATE TABLE public.gtest_child (
> f1 date NOT NULL,
> f2 bigint,
> f3 bigint GENERATED ALWAYS AS ((f2 * 3)) STORED
> );
>
> ALTER TABLE ONLY public.gtest_parent ATTACH PARTITION public.gtest_child FOR
> VALUES FROM ('2016-07-01') TO ('2016-08-01');
>
> and that fails at reload because the ATTACH PARTITION code path
> checks for equivalence of the generation expressions.
>
> This different-generated-expression situation isn't really morally
> different from different ordinary DEFAULT expressions, which we
> do endeavor to support.
Ah, right, we are a bit more flexible in allowing that. Though,
partition-specific defaults, unlike generated columns, are not
respected when inserting/updating via the parent:
create table partp (a int, b int generated always as (a+1) stored, c
int default 0) partition by list (a);
create table partc1 partition of partp (b generated always as (a+2)
stored, c default 1) for values in (1);
insert into partp values (1);
table partp;
a | b | c
---+---+---
1 | 3 | 0
(1 row)
create table partc2 partition of partp (b generated always as (a+2)
stored) for values in (2);
update partp set a = 2;
table partp;
a | b | c
---+---+---
2 | 4 | 0
(1 row)
> So maybe we should deem this a supported
> case and remove ATTACH PARTITION's insistence that the generation
> expressions match
I tend to agree now that we have 3f7836ff6.
> ... which I think would be a good thing anyway,
> because that check-for-same-reverse-compiled-expression business
> is pretty cheesy in itself.
Hmm, yeah, we usually transpose a parent's expression into one that
has a child's attribute numbers and vice versa when checking their
equivalence.
> AFAIK, 3f7836ff6 took care of the
> only problem that the backend would have with this, and pg_dump
> looks like it will work as long as the backend will take the
> ATTACH command.
Right.
> I also looked into making CREATE TABLE ... PARTITION OF reject
> this case; but that's much harder than it sounds, because what we
> have at the relevant point is a raw (unanalyzed) expression for
> the child's generation expression but a cooked one for the
> parent's, so there is no easy way to match the two.
>
> In short, it's seeming like the rule for both partitioning and
> traditional inheritance ought to be "a child column must have
> the same generated-or-not property as its parent, but their
> generation expressions need not be the same". Thoughts?
Agreed.
I've updated your disallow-generated-child-columns-2.patch to do this,
and have also merged the delta post that I had attached with my last
email, whose contents you sound to agree with.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
disallow-generated-child-columns-3.patch
Description: Binary data
