I'd like to discuss scaleout at PGCon

[MauMau]

Hello,

I'm going to attend PGCon in Ottawa for the first time.  I am happy if
I can meet you.

Because I'm visually impaired, I only have vision to sense light.  If
you see a Japanese man with a height of 171 cm with a white cane, it's
probably me.  I'd be happy if you talk to me.  But as I'm still far
from good at listening and speaking English, I'm sorry if I take an
unfriendly attitude or if I can not keep on talking for a long time.


I'd like to have a session on scaleout design at the unconference.
I've created a wiki page for that (this is still just a memo; I'd like
to populate this page with you as the discussion in the community
progresses).  I'd appreciate it if someone could stand with me and
facilitate the discussion at the unconference.

https://wiki.postgresql.org/wiki/Scaleout_Design

The background is ... our company is faced with an immediate need to
develop the read-write scaleout feature on PostgreSQL.  We tried
Postgres-XL with much hope, but we found it difficult to achieve our
performance goal.  I will tell you the details at the conference.  But
personally, Postgres-XL seems to be very nice software, and I feel
that good parts of it should be integrated into core.

I know that many great hackers from 2ndQuadrant, EnterpriseDB, NTT,
Postgres Professional, CitusData, and so on are addressing this
difficult scaleout feature.  I don't think yet we are competent to
lead this development.

On the other hand, we have a proprietary RDBMS called Symfoware (I'm
sure you don't know it), which is not based on PostgreSQL, that
provides the scaleout feature.  Its architecture is a mix of shared
nothing and shared everything.  It implements deadlock detection and
resolution without a central node or periodic monitoring, parallel 2PC
across nodes, parallel crash recovery, client connection routing and
failover without any overhead of intermediary middleware during SQL
execution, etc.  So we may be able to help in some way.  I'd be happy
if we could help the community to proceed with development of
scaleout.

If you have a session for scaleout outside the unconference, could you
call me and let me join it?


By the way, the popularity score of PostgreSQL finally exceeded 400
points in the DB-Engines ranking!  The popularity difference with the
top products has shrunk greatly.  Let's make PostgreSQL more popular.

https://db-engines.com/en/ranking

[as of May 27, 2018]
Oracle=1290.42  MySQL=1223.34  SQL Server=1085.84
PostgreSQL=400.90  MongoDB=342.11
(Oracle / PostgreSQL ratio is 3.2)

[as of Feb 2016, according to a memo at hand]
Oracle=1476.14  MySQL=1321.13  SQL Server=??
MongoDB=??  PostgreSQL=288.66
(Oracle / PostgreSQL ratio is 5.1)


Regards
MauMau

Re: Allowing printf("%m") only where it actually works

[Thomas Munro]

On Sun, May 27, 2018 at 12:38 PM, Tom Lane  wrote:
> At least in the case of ereport, all it takes to create a hazard is
> more than one sub-function, eg this is risky:
>
> ereport(..., errmsg(..., strerror(errno)), errdetail(...));
>
> because errdetail() might run first and malloc some memory for its
> constructed string.
>
> So I think a blanket policy of "don't trust errno within the arguments"
> is a good idea, even though it might be safe to violate it in the
> existing cases in exec.c.

Right, malloc() is a hazard I didn't think about.  I see that my local
malloc() makes an effort to save and restore errno around syscalls,
but even if all allocators were so thoughtful, which apparently they
aren't, there is also the problem that malloc itself can deliberately
set errno to ENOMEM per spec.  I take your more general point that you
can't rely on anything we didn't write not trashing errno, even libc.

On Tue, May 22, 2018 at 4:03 AM, Tom Lane  wrote:
> I noticed another can of worms here, too: on Windows, doesn't use of
> GetLastError() in elog/ereport have exactly the same hazard as errno?
> Or is there some reason to think it can't change value during errstart()?

Yeah, on Windows the same must apply, not in errstart() itself but any
time you pass more than one value to elog() using expressions that
call functions we can't audit for last-error-stomping.

Out of curiosity I tried adding a GetLastError variable for Windows
(to hide the function of that name and break callers) to the earlier
experimental patch (attached).  I had to give it an initial value to
get rid of a warning about an unused variable (by my reading of the
documentation, __pragma(warning(suppress:4101)) can be used in macros
(unlike #pragma) and should shut that warning up, but it doesn't work
for me, not sure why).  Of course that produces many errors since we
do that all over the place:

https://ci.appveyor.com/project/macdice/postgres/build/1.0.184

-- 
Thomas Munro
http://www.enterprisedb.com


prevent-errno-v2.patch
Description: Binary data

Re: Allowing printf("%m") only where it actually works

[Tom Lane]

I wrote:
> ... It doesn't take much to make one nontrivial either.  If
> memory serves, malloc() can trash errno on some platforms such as macOS,
> so even just a palloc creates a hazard of a hard-to-reproduce problem.

After digging around in the archives, the closest thing that we seem to
know for certain is that some versions of free() can trash errno, cf

https://www.postgresql.org/message-id/flat/E1UcmpR-0004nn-2i%40wrigleys.postgresql.org

as a result of possibly calling madvise() which might or might not
succeed.  But in the light of that knowledge, how much do you want
to bet that malloc() can't change errno?  And there's the possibility
that a called function does both a palloc and a pfree ...

regards, tom lane

Re: Allowing printf("%m") only where it actually works

[Tom Lane]

Thomas Munro  writes:
> On Sun, May 27, 2018 at 4:21 AM, Tom Lane  wrote:
>> (Basically what this would protect against is elog_start changing errno,
>> which it doesn't.)

> Hmm.  It looks like errstart() preserves errno to protect %m not from
> itself, but from the caller's other arguments to the elog facility.
> That seems reasonable, but do we really need to prohibit direct use of
> errno in expressions?  The only rogue actor likely to trash errno is
> you, the caller.  I mean, if you call elog(LOG, "foo %d %d", errno,
> fsync(bar)) it's obviously UB and your own fault, but who would do
> anything like that?  Or maybe I misunderstood the motivation.

Right, the concern is really about things like

elog(..., f(x), strerror(errno));

If f() trashes errno --- perhaps only some of the time --- then this
is problematic.  It's especially problematic because whether f() is
evaluated before or after strerror() is platform-dependent.  So even
if the original author had tested things thoroughly, it might break
for somebody else.

The cases in exec.c all seem safe enough, but we have lots of examples
in the backend where we call nontrivial functions in the arguments of
elog/ereport.  It doesn't take much to make one nontrivial either.  If
memory serves, malloc() can trash errno on some platforms such as macOS,
so even just a palloc creates a hazard of a hard-to-reproduce problem.

At least in the case of ereport, all it takes to create a hazard is
more than one sub-function, eg this is risky:

ereport(..., errmsg(..., strerror(errno)), errdetail(...));

because errdetail() might run first and malloc some memory for its
constructed string.

So I think a blanket policy of "don't trust errno within the arguments"
is a good idea, even though it might be safe to violate it in the
existing cases in exec.c.

regards, tom lane

Re: Allowing printf("%m") only where it actually works

[Thomas Munro]

On Sun, May 27, 2018 at 4:21 AM, Tom Lane  wrote:
> ...  So that seems like a rather high price to
> pay to deal with what, at present, is a purely hypothetical hazard.
> (Basically what this would protect against is elog_start changing errno,
> which it doesn't.)

Hmm.  It looks like errstart() preserves errno to protect %m not from
itself, but from the caller's other arguments to the elog facility.
That seems reasonable, but do we really need to prohibit direct use of
errno in expressions?  The only rogue actor likely to trash errno is
you, the caller.  I mean, if you call elog(LOG, "foo %d %d", errno,
fsync(bar)) it's obviously UB and your own fault, but who would do
anything like that?  Or maybe I misunderstood the motivation.

> Another approach we could consider is keeping exec.c's one-off approach
> to error handling and letting it redefine pg_prevent_errno_in_scope() as
> empty.  But that's ugly.
>
> Or we could make the affected call sites work like this:
>
> int save_errno = errno;
>
> log_error(_("could not identify current directory: %s"),
>   strerror(save_errno));
>
> which on the whole might be the most expedient thing.

That was what I was going to propose, until I started wondering why we
need to do anything here.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Redesigning the executor (async, JIT, memory efficiency)

[Andres Freund]

On 2018-05-26 17:08:51 -0400, Robert Haas wrote:
> On Fri, May 25, 2018 at 2:40 AM, Andres Freund  wrote:
> > I think we're going to have to continue showing the tree plan. I think
> > the linear version is far too hard to understand for anything
> > nontrivial.
>
> Some of that is because this format is intrinsically hard to read, but
> it's also partly because of unfamiliarity and partly because you
> haven't put much work into it.  You could certainly make it easier to
> read just by expending a little more effort on it:

Right - didn't seem like the highest priority thing to tackle. But
without displaying hierarchy even a lot of TLC won't make it easy to
understand.  Being able to quickly ignore, or even hide if you use
something like explain.depesz.com, entire subtrees is quite valuable.
We don't compose programs in one function + a lot of goto statements for
a reason...


> Even with this sort of TLC, I agree that reading this is no picnic,
> but I think continuing to show the tree structure when the executing
> plan has been linearized seems to have its own set of difficulties.

I'm not so sure. The plan would still be in a tree form, and I do think
realistically people do and should care more about the planner choices
rather than minute details of how things are actually executed.
Especially if we're going to whack that around on a granular level
repeatedly.


> Also, consider a plan that today looks like this:
>
> Append
> -> Seq Scan on p1
> -> Seq Scan on p2
> -> Seq Scan on p3
>
> The linearized plan could omit the Append node altogether.

Sure, but instrumentation nodes could continue to have a reference to
the Plan/Append node as their parent.


> How will a linearized plan handle handle backward fetches?

I don't quite know yet, TBH.  I'm playing with a few alternatives,
including:

a) Don't support it in general, require materialization at the top
   level. Has the advantage of minimal complexity that's only incurred
   when actually needed. And I think scrolling backwards is extremely
   rare in practice.
b) Emit a separate execution program the first time a different
   direction is required. But share enough state that the program can
   just be switched over.  That'd allow to remove all the necessary
   branches to support backward scan from the hot paths.
c) Continue as currently done, essentially.

Greetings,

Andres Freund

Re: SPI/backend equivalent of extended-query Describe(statement)?

[Andrew Gierth]

> "Chapman" == Chapman Flack  writes:

 >> So what I'm thinking now is that the way to go, if one wants to
 >> imitate the client-side protocol behavior closely, would be to have
 >> a setup hook that calls parse_variable_parameters the first time,
 >> and then parse_fixed_parameters on subsequent calls for
 >> revalidation.

 Chapman> What circumstances would call for revalidation, and what would
 Chapman> it be able to accomplish, under that design?

Any change that could invalidate the cached plan, such as altering any
of the tables referenced by it.

But thinking about it more (and actually trying it out in pllua-ng),
it's not as bad as I thought because parse_variable_parameters on
revalidation (at least the way I do it) will still be using the same
parameter types array as before, which will have the types of all
actually used parameters already filled in, and so those won't be
changed.

I think in the absence of a columnref hook, that means that the number
and type of parameters found when using parse_variable_parameters can't
change after the first time, so there's probably no need to get too
fancy in the hook.

-- 
Andrew (irc:RhodiumToad)

Re: Redesigning the executor (async, JIT, memory efficiency)

[Robert Haas]

On Fri, May 25, 2018 at 2:40 AM, Andres Freund  wrote:
> I think we're going to have to continue showing the tree plan. I think
> the linear version is far too hard to understand for anything
> nontrivial.

Some of that is because this format is intrinsically hard to read, but
it's also partly because of unfamiliarity and partly because you
haven't put much work into it.  You could certainly make it easier to
read just by expending a little more effort on it:

0: Initialize Sort (for step 7)
1: Initialize Seq Scan on lineitem (for step 2)
2: Iterate Seq Scan on linitem into slot 0
If no more tuples, go to step 5.
3: Test slot 0 using 
If failed, go to step 2.
4: Load Hash Aggregate from slot 0.
Go to step 2.
5: Retrieve from Hash Aggregate into slot 1.
If no more tuples, go to step 7.
etc.

Even with this sort of TLC, I agree that reading this is no picnic,
but I think continuing to show the tree structure when the executing
plan has been linearized seems to have its own set of difficulties.
The way that time is reported today in EXPLAIN ANALYZE presumes that
execute recurses down the tree; that's how times for higher nodes end
up including times for subordinate nodes.  If we time each step
executed using this method, that will no longer be true.  We could
make it true.  For example we could make the Seq Scan time include
steps 1, 2, and 3, the Hash Aggregate on top of it include that time
plus steps 4 and 5, the Sort on top of that also include steps 0, 6,
7, and 8, etc.  But that sucks.  If we've got timing information for
each step individually, that's awesome, and power users are going to
want to see it.  One of the most common questions one has when looking
at today's EXPLAIN ANALYZE output is "where did the time go?" and we
want to provide the most accurate possible answer to that question.

Also, consider a plan that today looks like this:

Append
-> Seq Scan on p1
-> Seq Scan on p2
-> Seq Scan on p3

The linearized plan could omit the Append node altogether.  We just
went and made Append nodes have a cost because they do, but in cases
where the Append is neither parallel-aware nor the new async-capable
Append type we're presumably going to add nor doing partition pruning,
we can and probably should optimize it away completely.  That will
change which plan is fastest in some cases, I'm fairly sure.  It
doesn't keep you from reporting on the Append node as if it still
existed, but if you do, you're sorta lying.  Another case that I think
is kind of interesting is:

Nested Loop Semi Join
-> Seq Scan
-> Index Scan
Index Cond: ...

If the index cond passes, we can emit the tuple returned by the Seq
Scan and jump back to fetch the next tuple from the Seq Scan.  Perhaps
the Nested Loop doesn't really exist at all.  If the join is something
other than a semi/anti join then you probably need the join to
project, unless all of the columns needed are from one side, but
that's all, unless the join itself has a Join Filter.  I think users
are going to want to see EXPLAIN ANALYZE output that makes it really
clear how the query has been optimized.  Sure, the existing format may
be easier for users in the short term, and certainly can be an option,
but seeing something that conceals rather than reveals how the
execution is really progressing may not be all that great in the end.

Maybe there are other ways of displaying the linearized plan that
would increase comprehensibility further.  For example, in your
version of the plan for TPC-H Q01, steps 0-1 are initialization, 2-4
are a loop, 5-6 are a loop, 7 is an independent step, 8-9 are another
loop, and 10 represents completion.  Maybe that can be used somehow to
make something easier to understand.  For example:

Initialize
-> Initialize Sort 1
-> Initiialize Seq Scan on lineitem
Loop
-> Iterate Seq Scan on lineitem
Break if no more tuples
-> Continue if ...
-> Load Into Hash Aggregate 1
Loop
-> Fetch From Hash Aggregate 1
-> Load Into Sort 1
Perform Sort 1
Loop
-> Fetch from Sort 1
-> Return Tuple

Maybe that doesn't seem that much easier, but at least it doesn't
involve needing to understand step numbers.  It strikes me that this
kind of organization could potentially also be useful for a query
progress indicator.  If the whole plan is a series of loops and other
steps, you can say things like -- well, the steps we've already
completed represent 30% of the estimated cost.  The steps we haven't
started yet represent another 30% of the estimated cost.  So we're
somewhere between 30% done and 70% done.  And then we can guess what
fraction of the iterations we've completed for the loop we're
currently performing.  Obviously this is all pie-in-the-sky, but it
certainly seems like this kind of thing would enable estimation that
we can't easily perform today.

How will a linearized plan handle handle backward fetches?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: found xmin from before relfrozenxid on pg_catalog.pg_authid

[Andres Freund]

On 2018-05-26 13:45:06 -0700, Andres Freund wrote:
> On 2018-05-25 15:05:31 -0700, Andres Freund wrote:
> > On 2018-05-25 17:47:37 -0400, Tom Lane wrote:
> > > For nailed indexes, we allow updating of some additional fields, and I
> > > guess what has to happen here is that we teach the code to update some
> > > additional fields for nailed tables too.
> > 
> > Yea, it seems like we could just get a new version of the pg_class tuple
> > if in the right state, and memcpy() it into place. Not sure if there's
> > any other issues...
> 
> That part isn't too hard. I've a patch that appears to address the
> issue, and isn't *too* ugly.
> 
> We don't really have a way to force .init file removal / update for
> shared relations however. Otherwise we'll just continue to read old data
> from .init files at startup. And there'll commonly not be any
> outstanding invalidation.  Thus it appears to me that we need to extend
> RelcacheInitFileInval to also support the shared file.  That's WAL
> logged, but it looks like we can just add flag like
> XACT_COMPLETION_UPDATE_RELCACHE_FILE without breaking the WAL format.
> 
> Does anybody see a way to not have to remove the .init file?

Just to be clear: We already remove the non-shared relcache init file
when a non-shared table in it is changed . Which I presume is the reason
this issue hasn't bitten us in a much bigger way. While the lack of
proper invalidations means that already running sessions will see the
wrong values and make wrong decisions, the fact that the non-shared file
will regularly be removed has reduced the impact quite a bit.

Greetings,

Andres Freund

Periods

[Vik Fearing]

SQL:2011 introduced the concept of a "period". It takes two existing columns 
and basically does the same thing as our range types except there is no new 
storage.  I believe if Jeff Davis had given us range types a few years later 
than he did, it would have been using periods.

Attached is a WIP patch that I have been working on.  The only thing left is 
completing periods on inherited tables, and finishing up pg_dump.  I'm posting 
this now just to make sure my basic foundation is sound, and to let people know 
that I'm working on this.

The patch itself doesn't have any functionality, it just allows periods to be 
defined.  With that, there are several things that we can do: SYSTEM_TIME 
periods, which are explicitly not allowed by this patch, will allow us to do 
SQL standard versioned tables, and also allows some time travel functionality.  
Application periods can be used in PRIMARY/UNIQUE keys, foreign keys, and give 
nice new features to UPDATE and DELETE.  They also allow "period predicates" 
which are the same kind of operations we already have for range types.  All of 
that is for future patches that build on the infrastructure presented in this 
patch.

The SQL standard restricts period columns to dates or timestamps, but I'm 
allowing anything that has a btree operator class, as is the PostgreSQL way. 
System periods, once allowed, will only be timestamptz though.  Unfortunately, 
I had to fully reserve the word PERIOD for this.

I'm looking for comments on everything except the pg_dump stuff, keeping in 
mind that inheritance is not finished either.

Thanks!

This is patch is based off of 71b349aef4.diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 3ed9021c2f..025d6b5355 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -225,6 +225,11 @@
   information about partition key of tables
  
 
+ 
+  pg_period
+  periods
+ 
+
  
   pg_pltemplate
   template data for procedural languages
@@ -4902,6 +4907,116 @@ SCRAM-SHA-256$:&l
  
 
 
+ 
+  pg_period
+
+  
+   pg_period
+  
+
+  
+   The catalog pg_period stores
+   information about system and application time periods.
+  
+
+  
+   Periods are described in .
+  
+
+  
+   pg_period Columns
+
+   
+
+ 
+  Name
+  Type
+  References
+  Description
+ 
+
+
+
+
+ 
+  pername
+  name
+  
+  Period name
+ 
+
+ 
+  perrelid
+  oid
+  pg_class.oid
+  The OID of the pg_class entry for the table containing this period.
+ 
+
+ 
+  perstart
+  int2
+  pg_attribute.attnum
+  
+   The attribute number of the start column.
+  
+ 
+
+ 
+  perend
+  int2
+  pg_attribute.attnum
+  
+   The attribute number of the end column.
+  
+ 
+
+ 
+  peropclass
+  oid
+  pg_opclass.oid
+  
+   This contains the OID of the operator class to use.
+  
+ 
+
+ 
+  perconstraint
+  oid
+  pg_constraint.oid
+  
+   This contains the OID of the CHECK constraint owned by the period to
+   ensure that (startcolumn
+   <
+   endcolumn).
+  
+ 
+
+ 
+  perislocal
+  bool
+  
+  
+   This period is defined locally for the relation.  Note that a period can
+   be locally defined and inherited simultaneously.
+  
+ 
+
+ 
+  perinhcount
+  int4
+  
+  
+   The number of direct inheritance ancestors this period has.  A period
+   with a nonzero number of ancestors cannot be dropped.
+  
+ 
+
+
+   
+  
+ 
+
+
  
   pg_pltemplate
 
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 2cd0b8ab9d..cdbe06196c 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -919,6 +919,64 @@ CREATE TABLE circles (
   
  
 
+ 
+  Periods
+
+  
+   Periods are definitions on a table that associate a period name with a start
+   column and an end column.  Both columns must be of exactly the same type
+   (including collation), have a btree operator class, and the start column
+   value must be strictly less than the end column value.
+  
+
+  
+   There are two types of periods: application and system.  System periods are
+   distinguished by their name which must be SYSTEM_TIME.  Any
+   other name is an application period.
+  
+
+  
+   Currently, periods in PostgreSQL have no functionality; they can only be
+   defined for future use.
+  
+
+  
+  Application Periods
+
+   
+period
+application
+   
+
+   
+Application periods are defined on a table using the following syntax:
+   
+
+
+CREATE TABLE billing_addresses (
+  customer_id integer,
+  address_id integer,
+  valid_from date,
+  valid_to date,
+  PERIOD FOR validity (valid_from, valid_to)
+);
+
+  
+
+  
+   System Periods
+
+   
+period
+system
+   
+
+   
+Periods for SYSTEM_TIME are currently not impleme

Re: found xmin from before relfrozenxid on pg_catalog.pg_authid

[Andres Freund]

On 2018-05-25 15:05:31 -0700, Andres Freund wrote:
> On 2018-05-25 17:47:37 -0400, Tom Lane wrote:
> > For nailed indexes, we allow updating of some additional fields, and I
> > guess what has to happen here is that we teach the code to update some
> > additional fields for nailed tables too.
> 
> Yea, it seems like we could just get a new version of the pg_class tuple
> if in the right state, and memcpy() it into place. Not sure if there's
> any other issues...

That part isn't too hard. I've a patch that appears to address the
issue, and isn't *too* ugly.

We don't really have a way to force .init file removal / update for
shared relations however. Otherwise we'll just continue to read old data
from .init files at startup. And there'll commonly not be any
outstanding invalidation.  Thus it appears to me that we need to extend
RelcacheInitFileInval to also support the shared file.  That's WAL
logged, but it looks like we can just add flag like
XACT_COMPLETION_UPDATE_RELCACHE_FILE without breaking the WAL format.

Does anybody see a way to not have to remove the .init file?

Greetings,

Andres Freund

Re: SPI/backend equivalent of extended-query Describe(statement)?

[Chapman Flack]

On 05/26/18 15:22, Andrew Gierth wrote:
> So what I'm thinking now is that the way to go, if one wants to imitate
> the client-side protocol behavior closely, would be to have a setup hook
> that calls parse_variable_parameters the first time, and then
> parse_fixed_parameters on subsequent calls for revalidation.

What circumstances would call for revalidation, and what would it be
able to accomplish, under that design?

I'm kind of trying to think out what the semantics could be in PL/Java,
should they be changed from what they are now (which is just to tell you
all params are varchar until you have supplied values for them, and then
tell you what types you supplied, in case you'd forgotten).

It's interesting that with parse_variable_parameters, it is possible
to supply some types explicitly, while leaving others to be inferred.
In one plausible implementation, that could be what would happen in
PL/Java if you called prepareStatement(), then called setter methods
on a few of the parameters, then called getParameterMetaData(). And
I suppose there could be cases where the explicitly supplied types for
some parameters would affect the types that get inferred for others.

On one hand, that sounds like it could be a bit confusing, and on the
other, sounds like it might be useful sometimes and I should just embrace
my inner Schrödinger and say yeah, that's how it works, you 'collapse'
the type assignments to whatever fits best at whatever moment you call
getParameterMetaData().

>From a glance at the (network client) pgjdbc code, that seems to be
what you'd expect to happen there, too; it gets sent to the backend
for "describe only" at the moment getParameterMetaData is called.

What I can find of the JDBC spec seems informal and breezy about this
stuff to an unsettling degree

-Chap

Re: SPI/backend equivalent of extended-query Describe(statement)?

[Andrew Gierth]

> "Chapman" == Chapman Flack  writes:

 >> Really our hook mechanism only supports adding hooks, not removing
 >> them.

 Chapman> I suppose the pllua_spi_prepare_checkparam_hook could be
 Chapman> linked in once and for all, and turned on and off just where
 Chapman> the code now hooks and unhooks it, and just forward to the
 Chapman> next hook when it's off.

Yeah, or have it detect whether the ParseState it's being called for is
ours by some other means.

 >> I'm not following why that's such a problem?  The whole point of
 >> SPI_prepare_params and friends is that the actual number and types
 >> of the parameters is hidden behind the parse hooks and ParamListInfo
 >> --- and, indeed, could change from one execution to the next.

So while looking at the hook issue, I found another can of worms.

What a protocol-level Parse does is to call parse-analysis via
parse_analyze_varparams, which calls parse_variable_parameters _without_
making it a parser setup hook (either there or in CompleteCachedPlan).

This has the effect of casting the parameter types in stone on the first
parse, as the client expects; a subsequent revalidate of the statement
will use pg_analyze_and_rewrite, which takes a fixed parameter list.

However if you call parse_variable_parameters from a hook passed to
SPI_prepare_params, then you're asking for it to be called again on
revalidations, which means that the parameters might change (even if
just changing types, I think you'd need a more complex set of hooks than
parse_variable_parameters uses to change the number of parameters too).

So what I'm thinking now is that the way to go, if one wants to imitate
the client-side protocol behavior closely, would be to have a setup hook
that calls parse_variable_parameters the first time, and then
parse_fixed_parameters on subsequent calls for revalidation.

-- 
Andrew (irc:RhodiumToad)

Re: Adding a new table to the system catalog

[Tom Lane]

Paul Howells  writes:
> I am exploring and poking at the source code to inform a design for adding
> valid-time support to postgres.  The feature will required updating
> existing system catalog tables as well as adding one or more new tables.

> Is there any documentation on how to update the system catalog?  What files
> need to be created/updated?  Do I need to run any utilities for generation
> manually or is this done as part of the build?  How do I assign a new OID?

The traditional advice for this is to look at past patches that added new
catalogs.  Recent examples that look pretty small otherwise include commit
1753b1b02 ("Add pg_sequence system catalog") and commit 6c268df12 ("Add
new catalog called pg_init_privs").  Also look at patches that just added
a column to an existing catalog, since it sounds like you need to do that
too; perhaps 039eb6e92 ("Logical replication support for TRUNCATE").

However, if you're doing this against HEAD, as you should be if you harbor
any hopes of getting a patch accepted in the future, you need to be aware
that we just made drastic changes in the way we represent initial catalog
data (i.e., entries inserted during bootstrap).  The old way with DATA
lines in the catalog/*.h files is gone in favor of *.dat files.  The good
news is that it's better documented than before, and we got rid of some of
the more tedious manual steps like creating Anum_foo macros.  See

https://www.postgresql.org/docs/devel/static/bki.html

and be sure to study the current contents of the catalog/*.h files
rather than what was there up to a couple months ago.

regards, tom lane

Re: Allowing printf("%m") only where it actually works

[Tom Lane]

I wrote:
> Thomas Munro  writes:
>> Here's an experimental way to do that, if you don't mind depending on
>> gory details of libc implementations (ie knowledge of what it expands
>> too).  Not sure how to avoid that since it's a macro on all modern
>> systems, and we don't have a way to temporarily redefine a macro.  If
>> you enable it for just ereport(), it compiles cleanly after 81256cd
>> (but fails on earlier commits).  If you enable it for elog() too then
>> it finds problems with exec.c.

> Hmm ... that's pretty duff code in exec.c, isn't it.  Aside from the
> question of errno unsafety, it's using elog where it really ought to be
> using ereport, it's not taking any thought for the reported SQLSTATE,
> etc.  I'm hesitant to mess with it mere hours before the beta wrap,
> but we really oughta improve that.

I wrote up a patch that makes src/common/exec.c do error reporting more
like other frontend/backend-common files (attached).  Now that I've done
so, though, I'm having second thoughts.  The thing that I don't like
about this is that it doubles the number of translatable strings created
by this file.  While there's not *that* many of them, translators have to
deal with each one several times because this file is included by several
different frontend programs.  So that seems like a rather high price to
pay to deal with what, at present, is a purely hypothetical hazard.
(Basically what this would protect against is elog_start changing errno,
which it doesn't.)  Improving the errcode situation is somewhat useful,
but still maybe it's not worth the cost.

Another approach we could consider is keeping exec.c's one-off approach
to error handling and letting it redefine pg_prevent_errno_in_scope() as
empty.  But that's ugly.

Or we could make the affected call sites work like this:

int save_errno = errno;

log_error(_("could not identify current directory: %s"),
  strerror(save_errno));

which on the whole might be the most expedient thing.

regards, tom lane

diff --git a/src/common/exec.c b/src/common/exec.c
index 4df16cd..f0d52e1 100644
*** a/src/common/exec.c
--- b/src/common/exec.c
***
*** 25,40 
  #include 
  #include 
  
- #ifndef FRONTEND
- /* We use only 3- and 4-parameter elog calls in this file, for simplicity */
- /* NOTE: caller must provide gettext call around str! */
- #define log_error(str, param)	elog(LOG, str, param)
- #define log_error4(str, param, arg1)	elog(LOG, str, param, arg1)
- #else
- #define log_error(str, param)	(fprintf(stderr, str, param), fputc('\n', stderr))
- #define log_error4(str, param, arg1)	(fprintf(stderr, str, param, arg1), fputc('\n', stderr))
- #endif
- 
  #ifdef _MSC_VER
  #define getcwd(cwd,len)  GetCurrentDirectory(len, cwd)
  #endif
--- 25,30 
*** find_my_exec(const char *argv0, char *re
*** 124,131 
  
  	if (!getcwd(cwd, MAXPGPATH))
  	{
! 		log_error(_("could not identify current directory: %s"),
!   strerror(errno));
  		return -1;
  	}
  
--- 114,127 
  
  	if (!getcwd(cwd, MAXPGPATH))
  	{
! #ifndef FRONTEND
! 		ereport(LOG,
! (errcode_for_file_access(),
!  errmsg("could not identify current directory: %m")));
! #else
! 		fprintf(stderr, _("could not identify current directory: %s\n"),
! strerror(errno));
! #endif
  		return -1;
  	}
  
*** find_my_exec(const char *argv0, char *re
*** 143,149 
  		if (validate_exec(retpath) == 0)
  			return resolve_symlinks(retpath);
  
! 		log_error(_("invalid binary \"%s\""), retpath);
  		return -1;
  	}
  
--- 139,149 
  		if (validate_exec(retpath) == 0)
  			return resolve_symlinks(retpath);
  
! #ifndef FRONTEND
! 		ereport(LOG, (errmsg("invalid binary \"%s\"", retpath)));
! #else
! 		fprintf(stderr, _("invalid binary \"%s\"\n"), retpath);
! #endif
  		return -1;
  	}
  
*** find_my_exec(const char *argv0, char *re
*** 192,205 
  case -1:		/* wasn't even a candidate, keep looking */
  	break;
  case -2:		/* found but disqualified */
! 	log_error(_("could not read binary \"%s\""),
! 			  retpath);
  	break;
  			}
  		} while (*endp);
  	}
  
! 	log_error(_("could not find a \"%s\" to execute"), argv0);
  	return -1;
  }
  
--- 192,214 
  case -1:		/* wasn't even a candidate, keep looking */
  	break;
  case -2:		/* found but disqualified */
! #ifndef FRONTEND
! 	ereport(LOG, (errmsg("could not read binary \"%s\"",
! 		 retpath)));
! #else
! 	fprintf(stderr, _("could not read binary \"%s\"\n"),
! 			retpath);
! #endif
  	break;
  			}
  		} while (*endp);
  	}
  
! #ifndef FRONTEND
! 	ereport(LOG, (errmsg("could not find a \"%s\" to execute", argv0)));
! #else
! 	fprintf(stderr, _("could not find a \"%s\" to execute\n"), argv0);
! #endif
  	return -1;
  }
  
*** resolve_symlinks(char *path)
*** 238,245 
  	 */
  	if (!getcwd(orig_wd, MAXPGPATH))
  	{
! 		log_error(_("could not identify c

Re: SPI/backend equivalent of extended-query Describe(statement)?

[Chapman Flack]

On 05/26/18 10:03, Tom Lane wrote:
> Really our hook mechanism only supports adding hooks, not removing them.

I suppose the pllua_spi_prepare_checkparam_hook could be linked in once
and for all, and turned on and off just where the code now hooks and
unhooks it, and just forward to the next hook when it's off.

>> Yeah. Another issue I ran into is that if you use SPI_prepare_params,
>> then you have to use SPI_execute_plan_with_paramlist, it's not possible
>> to use SPI_execute_plan (or SPI_execute_snapshot) instead because
>> plan->nargs was set to 0 by the prepare and never filled in with the
>> actual parameter count.
> 
> I'm not following why that's such a problem?  The whole point of
> SPI_prepare_params and friends is that the actual number and types
> of the parameters is hidden behind the parse hooks and ParamListInfo
> --- and, indeed, could change from one execution to the next.
> SPI_execute_plan only makes sense with a predetermined, fixed
> parameter list.

Well, when you're implementing a PL, you're faced with this task of
mapping/coercing parameters from a PL type system that invariably
(invariably? Yes, I think, unless your PL's name has "sql" in it)
differs from SQL's, and that's a pretty ill-defined task if your PL
runtime is handed a query to prepare in the form of a string, and
then handed some parameter values in the PL's type system, and can't
find out what SQL types they could appropriately be mapped/coerced to
for the query to be valid.

In the JDBC API, you pass a query string to prepareStatement(), and
then on what you get back you can call getParameterMetaData() and
learn what SQL thinks the types of the parameters will have to be.
That isn't really expected to change; the requirement isn't necessarily
to support some dizzying all-dynamic-all-the-time usage pattern, it's
just to be able to get the information, and SPI_prepare_params seems
the only way to get it.

-Chap

Adding a new table to the system catalog

[Paul Howells]

Hello All,

I am exploring and poking at the source code to inform a design for adding
valid-time support to postgres.  The feature will required updating
existing system catalog tables as well as adding one or more new tables.

Is there any documentation on how to update the system catalog?  What files
need to be created/updated?  Do I need to run any utilities for generation
manually or is this done as part of the build?  How do I assign a new OID?

Thanks in advance
Paul

Re: [PATCH] Clear up perlcritic 'missing return' warning

[Andrew Dunstan]

On 05/23/2018 02:00 PM, Andrew Dunstan wrote:

On Wed, May 23, 2018 at 1:45 PM, Alvaro Herrera
 wrote:

On 2018-May-23, Andrew Dunstan wrote:


And yes, the idea is that if we do this then we adopt a perlcritic
policy that calls it out when we forget.

If we can have a buildfarm animal that runs perlcritic that starts green
(and turns yellow with any new critique), with a config that's also part
of our tree, so we can easily change it as we see fit, it should be
good.



Should be completely trivial to do. We already have the core
infrastructure - I added it a week or two ago.




Not quite trivial but it's done - see 
.


crake is now set up to run this - see 



So, are there any other objections?

The patch Mike supplied doesn't give us a clean run (at least on the 
machine I tested on), since it turns down the severity level to 4 but 
leaves some items unfixed. I propose to enable this policy at level 5 
for now, and then remove that when we can go down to level 4 cleanly, 
and use its default setting at that stage.


cheers

andrew

--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: jsonb iterator not fully initialized

[Andrew Dunstan]

On 05/26/2018 03:09 AM, Piotr Stefaniak wrote:

On 2018-05-26 02:02, Peter Eisentraut wrote:

I got this error message via -fsanitized=undefined:

jsonfuncs.c:5169:12: runtime error: load of value 127, which is not a
valid value for type '_Bool'

The query was

select ts_headline('{}'::jsonb, tsquery('aaa & bbb'));

This calls the C function ts_headline_jsonb_byid_opt(), which calls
transform_jsonb_string_values(), which calls

  it = JsonbIteratorInit(&jsonb->root);
  is_scalar = it->isScalar;

but it->isScalar is not always initialized by JsonbIteratorInit().  (So
the 127 is quite likely clobbered memory.)

It can be fixed this way:

--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -901,7 +901,7 @@ iteratorFromContainer(JsonbContainer *container,
JsonbIterator *parent)
   {
  JsonbIterator *it;

-   it = palloc(sizeof(JsonbIterator));
+   it = palloc0(sizeof(JsonbIterator));
  it->container = container;
  it->parent = parent;
  it->nElems = JsonContainerSize(container);

It's probably not a problem in practice, since the isScalar business is
apparently only used in the array case, but it's dubious to leave things
uninitialized like this nonetheless.


I've seen it earlier but couldn't decide what my proposed fix should
look like. One of the options I considered was:

--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -5010,10 +5010,8 @@ transform_jsonb_string_values(Jsonb *jsonb, void
*action_state,
  JsonbIteratorToken type;
  JsonbParseState *st = NULL;
  text   *out;
-   boolis_scalar = false;

  it = JsonbIteratorInit(&jsonb->root);
-   is_scalar = it->isScalar;

  while ((type = JsonbIteratorNext(&it, &v, false)) != WJB_DONE)
  {
@@ -5033,7 +5031,7 @@ transform_jsonb_string_values(Jsonb *jsonb, void
*action_state,
  }

  if (res->type == jbvArray)
-   res->val.array.rawScalar = is_scalar;
+   res->val.array.rawScalar =
JsonContainerIsScalar(&jsonb->root);

  return JsonbValueToJsonb(res);
   }



palloc0 seems cleaner.

cheers

andrew

--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Sat, May 26, 2018 at 09:08:50AM -0400, Bruce Momjian wrote:
> On Sat, May 26, 2018 at 08:32:20AM +0900, Michael Paquier wrote:
>>
>> OK, I can live with that as well.  So we'll go in the direction of two
>> parameters then:
>> - scram_channel_binding, which can use "prefer" (default), "require" or
>> "disable".
>> - scram_channel_binding_name, developer option to choose the type of
>> channel binding, with "tls-unique" (default) and "tls-server-end-point".
>> We could also remove the prefix "scram_".  Ideas of names are welcome.
> 
> scram_channel_binding_method?

Or scram_channel_binding_type.  The first sentence of RFC 5929 uses this
term.

> Do we really know someone is going to want to actually specify the
> channel binding type?  If it is only testing, maybe we don't need to
> document this parameter.

Keeping everything documented is useful as well for new developers, as
they need to guess less from the code.  So I would prefer seeing both
connection parameters documented, and mentioning directly in the docs if
a parameter is for developers or not.
--
Michael


signature.asc
Description: PGP signature

Re: Avoiding Tablespace path collision for primary and standby

[Tom Lane]

Thomas Munro  writes:
> I also wondered about this when trying to figure out how to write a
> TAP test for recovery testing with tablespaces, for my undo proposal.
> I was starting to wonder about either allowing relative paths or
> supporting some kind of variable in the tablespace path that could
> then be set differently in each cluster's .conf.

Yeah, the configuration-variable solution had occurred to me too.
I'm not sure how convenient it'd be in practice, but perhaps it
would be workable.

Not sure about the relative-path idea.  Seems like that would create
a huge temptation to put tablespaces inside the data directory, which
would force us to deal with that can of worms.  Also, to the extent
that people use tablespaces for what they're actually meant to be
used for (ie, putting some stuff into a different filesystem), I can't
see a relative path being helpful.  Admins don't go mounting disks
at random places in the filesystem tree.

regards, tom lane

Re: SPI/backend equivalent of extended-query Describe(statement)?

[Tom Lane]

Andrew Gierth  writes:
> "Tom" == Tom Lane  writes:
>  Tom> Yikes.  That seems pretty unsafe :-(

> I put in a recursion check out of paranoia, but even after considerable
> thought wasn't able to figure out any scenario that would actually break
> it. If it's actually unsafe I'd really like to know about it :-)

The worrisome thing is the possibility that some other extension tries
to add to (or otherwise change) the post_parse_analyze_hook list while
you have it in a temporary state.  I can't think of a really likely
scenario for that, because I don't think parse analysis would ever cause
loading of a shared library that wasn't loaded already ... but just to
state that assumption is to expose how non-future-proof it is.

Really our hook mechanism only supports adding hooks, not removing them.

>  Tom> Obviously, I missed a bet by not folding check_variable_parameters
>  Tom> into the pstate hook mechanism. It's a bit late to do anything
>  Tom> about that for v11, but I'd favor trying to improve the situation
>  Tom> in v12.

> Yeah. Another issue I ran into is that if you use SPI_prepare_params,
> then you have to use SPI_execute_plan_with_paramlist, it's not possible
> to use SPI_execute_plan (or SPI_execute_snapshot) instead because
> plan->nargs was set to 0 by the prepare and never filled in with the
> actual parameter count.

I'm not following why that's such a problem?  The whole point of
SPI_prepare_params and friends is that the actual number and types
of the parameters is hidden behind the parse hooks and ParamListInfo
--- and, indeed, could change from one execution to the next.
SPI_execute_plan only makes sense with a predetermined, fixed
parameter list.

regards, tom lane

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Sat, May 26, 2018 at 08:32:20AM +0900, Michael Paquier wrote:
> On Fri, May 25, 2018 at 06:24:07PM +0300, Heikki Linnakangas wrote:
> > On 25 May 2018 17:44:16 EEST, Robert Haas  wrote:
> >> It seems to me that this is really another sort of thing altogether.
> >> Whether or not you want to insist on channel binding is a completely
> >> separate thing from which channel binding methods you're willing to
> >> use.  It seems to me like the most logical thing would be to make
> >> these two separate connection options. 
> > 
> > Works for me.
> 
> OK, I can live with that as well.  So we'll go in the direction of two
> parameters then:
> - scram_channel_binding, which can use "prefer" (default), "require" or
> "disable".
> - scram_channel_binding_name, developer option to choose the type of
> channel binding, with "tls-unique" (default) and "tls-server-end-point".
> We could also remove the prefix "scram_".  Ideas of names are welcome.

scram_channel_binding_method?

> At the end, the core of the proposed patch relies on the fact that it
> checks when receiving AUTH_REQ_OK that a full set of SCRAM messages has
> happened with the server, up to the point where the client has checked
> the server proof that both ends know the same password.  So do people
> here think that this is a sane apprach?  Are there other ideas?

Do we really know someone is going to want to actually specify the
channel binding type?  If it is only testing, maybe we don't need to
document this parameter.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: zheap: a new storage format for PostgreSQL

[Amit Kapila]

On Fri, Mar 2, 2018 at 4:05 PM, Alexander Korotkov
 wrote:
> On Fri, Mar 2, 2018 at 1:31 PM, Amit Kapila  wrote:
>>
>> On Fri, Mar 2, 2018 at 1:50 PM, Tsunakawa, Takayuki
>>  wrote:
>> > From: Amit Kapila [mailto:amit.kapil...@gmail.com]
>> >> At EnterpriseDB, we (me and some of my colleagues) are working from
>> >> more
>> >> than a year on the new storage format in which only the latest version
>> >> of
>> >> the data is kept in main storage and the old versions are moved to an
>> >> undo
>> >> log.  We call this new storage format "zheap".  To be clear, this
>> >> proposal
>> >> is for PG-12.
>> >
>> > Wonderful!  BTW, what "z" stand for?  Ultimate?
>> >
>>
>> There is no special meaning to 'z'.  We have discussed quite a few
>> names (like newheap, nheap, zheap and some more on those lines), but
>> zheap sounds better.  IIRC, one among Robert or Thomas has come up
>> with this name.
>
>
> I would propose "zero-bloat heap" disambiguation of zheap.  Seems like fair
> enough explanation for me without need to rename :)
>

It's been a while since we have updated the progress on this project,
so here is an update.  This is based on the features that were not
working (as mentioned in Readme.md) when the branch was published.
1. TID Scans are working now.
2. Insert .. On Conflict is working now.
3. Tuple locking is working with a restriction that if there are more
concurrent lockers on a page than the number of transaction slots on a
page, then some of the lockers will wait till others get committed.
We are working on a solution to extend the number of transaction slots
on a separate set of pages which exist in heap, but will contain only
transaction data.  There are also some corner cases where it doesn't
work for Rollbacks.
4. Foreign keys are working.
5. Vacuum/Autovacuum is working.
6. Rollback prepared transactions.

Apart from this, we have fixed some other open issues.  I think to
discuss some of the designs, we need to start separate threads (like
Thomas has already started a thread on undo logs[1]), but it is also
okay to discuss on this thread as well.  One specific thing where we
need some input is about testing of this new heap.  As of now, the
idea we are using to test it is by having a guc parameter
(storage_engine) which if set to zheap, all the regression tests will
create tables in zheap and the operations are zheap specific.  This
basically works okay, but the results are different than expected in
some cases like (a) in-place updates cause rows to be printed in
different order (b) ctid based tests gives different results because
zheap has a metapage and TPD pages, (c) \d+ show storage_engine as an
option, etc.  We workaround it by either creating a separate .out file
for zheap or sometimes by masking the expected different output (like
we don't allow to compare additional storage_engine option as output
of \d+).  I know this is not the best way to test a new storage
engine, but for now it helped us a lot.  I think we need some generic
way to test new storage engines.  I am not sure if it good to discuss
it here or does this belong to Pluggable API thread.

Any thoughts?

[1] - 
https://www.postgresql.org/message-id/CAEepm%3D2EqROYJ_xYz4v5kfr4b0qw_Lq_6Pe8RTEC8rx3upWsSQ%40mail.gmail.com

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: jsonb iterator not fully initialized

[Piotr Stefaniak]

On 2018-05-26 02:02, Peter Eisentraut wrote:
> I got this error message via -fsanitized=undefined:
> 
> jsonfuncs.c:5169:12: runtime error: load of value 127, which is not a
> valid value for type '_Bool'
> 
> The query was
> 
> select ts_headline('{}'::jsonb, tsquery('aaa & bbb'));
> 
> This calls the C function ts_headline_jsonb_byid_opt(), which calls
> transform_jsonb_string_values(), which calls
> 
>  it = JsonbIteratorInit(&jsonb->root);
>  is_scalar = it->isScalar;
> 
> but it->isScalar is not always initialized by JsonbIteratorInit().  (So
> the 127 is quite likely clobbered memory.)
> 
> It can be fixed this way:
> 
> --- a/src/backend/utils/adt/jsonb_util.c
> +++ b/src/backend/utils/adt/jsonb_util.c
> @@ -901,7 +901,7 @@ iteratorFromContainer(JsonbContainer *container,
> JsonbIterator *parent)
>   {
>  JsonbIterator *it;
> 
> -   it = palloc(sizeof(JsonbIterator));
> +   it = palloc0(sizeof(JsonbIterator));
>  it->container = container;
>  it->parent = parent;
>  it->nElems = JsonContainerSize(container);
> 
> It's probably not a problem in practice, since the isScalar business is
> apparently only used in the array case, but it's dubious to leave things
> uninitialized like this nonetheless.
> 

I've seen it earlier but couldn't decide what my proposed fix should 
look like. One of the options I considered was:

--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -5010,10 +5010,8 @@ transform_jsonb_string_values(Jsonb *jsonb, void 
*action_state,
 JsonbIteratorToken type;
 JsonbParseState *st = NULL;
 text   *out;
-   boolis_scalar = false;

 it = JsonbIteratorInit(&jsonb->root);
-   is_scalar = it->isScalar;

 while ((type = JsonbIteratorNext(&it, &v, false)) != WJB_DONE)
 {
@@ -5033,7 +5031,7 @@ transform_jsonb_string_values(Jsonb *jsonb, void 
*action_state,
 }

 if (res->type == jbvArray)
-   res->val.array.rawScalar = is_scalar;
+   res->val.array.rawScalar = 
JsonContainerIsScalar(&jsonb->root);

 return JsonbValueToJsonb(res);
  }