[HACKERS] Run pgindent now?

2015-05-16 Thread Tom Lane
With feature freeze behind us, I'd like to propose that now is a good
time for a pgindent run.  It's possible we'd need another one before
9.5 is branched off from HEAD, but a run now ought to take care of 95%
of the cleanup needed.  I see a couple of advantages to doing it now:

1. Patches that are now postponed to 9.6 will need to be rebased against
pgindented sources anyway.  Might as well give their authors more time
for that rather than less.

2. Code that matches the project layout conventions is easier to read
and review, IMO (not that I'm especially in love with the pgindent
layout, but I'm used to it).  Also any issues like commit d678bde65
would be taken care of, which is an objective reason why reviewing is
easier.

The only significant downside I can think of is that, if we determine
that any of the recent feature additions are so sketchy that they need
to be reverted, having to undo just a portion of the pgindent commit
before reverting the feature commit would be a pain.  But I don't think
we should optimize on the assumption that that will happen.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Run pgindent now?

2015-05-16 Thread Tom Lane
Magnus Hagander mag...@hagander.net writes:
 On Sat, May 16, 2015 at 5:58 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 With feature freeze behind us, I'd like to propose that now is a good
 time for a pgindent run.

 +1, except I suggest we at least delay it until we have wrapped the new
 minor releases, to make sure we don't conflict with any backpatching there.

Sure, a couple of days won't make much difference.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pg_upgrade cleanup

2015-05-16 Thread Bruce Momjian
On Sat, May 16, 2015 at 12:21:12PM -0400, Noah Misch wrote:
 On Thu, May 14, 2015 at 10:06:11PM -0400, Bruce Momjian wrote:
  On Thu, May 14, 2015 at 09:56:53PM -0400, Bruce Momjian wrote:
   This patch makes pg_upgrade controldata checks more consistent, and adds
   a missing check for float8_pass_by_value.
  
  Sorry, I should have mentioned I applied this patch to head.  It isn't
  significant enough to backpatch.
 
 A float8_pass_by_value match is unnecessary, and requiring it creates needless
 hassle for users.  Switching between USE_FLOAT8_BYVAL binaries and
 !USE_FLOAT8_BYVAL binaries requires an initdb to get different values in
 pg_type.typbyval and pg_attribute.attbyval.  pg_upgrade's use of pg_dump to
 migrate catalog content addresses that fine.  Note that
 check_for_isn_and_int8_passing_mismatch() exists because pg_upgrade has
 allowed source and destination clusters to differ in USE_FLOAT8_BYVAL.

What we had was checking for float8_pass_by_value, but did nothing with
it, so I assumed we had lost the check somehow.  I will remove detecting
and checking of that value.  Thanks.

 The rest of this change is opaque to me.  More consistent with what?  The
 old use of the tli variable sure looked like a bug, considering the variable
 was never set to anything but zero.  What is the anticipated behavior change?

The problem was that the option checking was not in a consistent order,
so there was no easy easy to make sure everything was being processed
properly.  The new ordering is consistent.

I thought the tli was a harmless cleanup but I now see it was passing a
zero timeline to pg_resetxlog.  The only reason that worked was because
pg_resetxlog ignores a timeline that is less than our current one, and
zero was always less than the timeline so pg_resetxlog was making no
timeline change at all.  I will clean that up too in backbranches as it
is to odd.  (I think it was broken by
038f3a05092365eca070bdc588554520dfd5ffb9).

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

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Run pgindent now?

2015-05-16 Thread Magnus Hagander
On Sat, May 16, 2015 at 5:58 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 With feature freeze behind us, I'd like to propose that now is a good
 time for a pgindent run.  It's possible we'd need another one before
 9.5 is branched off from HEAD, but a run now ought to take care of 95%
 of the cleanup needed.  I see a couple of advantages to doing it now:

 1. Patches that are now postponed to 9.6 will need to be rebased against
 pgindented sources anyway.  Might as well give their authors more time
 for that rather than less.

 2. Code that matches the project layout conventions is easier to read
 and review, IMO (not that I'm especially in love with the pgindent
 layout, but I'm used to it).  Also any issues like commit d678bde65
 would be taken care of, which is an objective reason why reviewing is
 easier.

 The only significant downside I can think of is that, if we determine
 that any of the recent feature additions are so sketchy that they need
 to be reverted, having to undo just a portion of the pgindent commit
 before reverting the feature commit would be a pain.  But I don't think
 we should optimize on the assumption that that will happen.


+1, except I suggest we at least delay it until we have wrapped the new
minor releases, to make sure we don't conflict with any backpatching there.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


Re: [HACKERS] Support for N synchronous standby servers - take 2

2015-05-16 Thread Michael Paquier
On Sat, May 16, 2015 at 5:58 PM, Sawada Masahiko wrote:
 The dedicated language for multiple sync replication would be more
 extensibility as you said, but I think there are not a lot of user who
 want to or should use this.
 IMHO such a dedicated extensible feature could be extension module,
 i.g. contrib. And we could implement more simpler feature into
 PostgreSQL core with some restriction.

As proposed, this feature does not bring us really closer to quorum
commit, and AFAIK this is what we are more or less aiming at recalling
previous discussions. Particularly with the syntax proposed above, it
is not possible to do some OR conditions on subgroups of nodes, the
list of nodes is forcibly using AND because it is necessary to wait
for all the subgroups. Also, users may want to track nodes from the
same group with different application_name.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Run pgindent now?

2015-05-16 Thread Noah Misch
On Sat, May 16, 2015 at 11:58:59AM -0400, Tom Lane wrote:
 With feature freeze behind us, I'd like to propose that now is a good
 time for a pgindent run.  It's possible we'd need another one before
 9.5 is branched off from HEAD, but a run now ought to take care of 95%
 of the cleanup needed.  I see a couple of advantages to doing it now:

+1 to Magnus's suggestion of doing it after the minor release wrap.

 The only significant downside I can think of is that, if we determine
 that any of the recent feature additions are so sketchy that they need
 to be reverted, having to undo just a portion of the pgindent commit
 before reverting the feature commit would be a pain.  But I don't think
 we should optimize on the assumption that that will happen.

Quite so; we'd have lost our way to be optimizing for that.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] hstore_plpython regression test does not work on Python 3

2015-05-16 Thread Tom Lane
As exhibited for instance here:

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=spoonbilldt=2015-05-16%2011%3A00%3A07

I've been able to replicate this on a Fedora 21 box: works fine with
Python 2, fails with Python 3.  Seems like we still have an issue
with reliance on a system-provided sort method.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pg_upgrade cleanup

2015-05-16 Thread Bruce Momjian
On Sat, May 16, 2015 at 02:49:08PM -0400, Bruce Momjian wrote:
 On Sat, May 16, 2015 at 12:21:12PM -0400, Noah Misch wrote:
  On Thu, May 14, 2015 at 10:06:11PM -0400, Bruce Momjian wrote:
   On Thu, May 14, 2015 at 09:56:53PM -0400, Bruce Momjian wrote:
This patch makes pg_upgrade controldata checks more consistent, and adds
a missing check for float8_pass_by_value.
   
   Sorry, I should have mentioned I applied this patch to head.  It isn't
   significant enough to backpatch.
  
  A float8_pass_by_value match is unnecessary, and requiring it creates 
  needless
  hassle for users.  Switching between USE_FLOAT8_BYVAL binaries and
  !USE_FLOAT8_BYVAL binaries requires an initdb to get different values in
  pg_type.typbyval and pg_attribute.attbyval.  pg_upgrade's use of pg_dump to
  migrate catalog content addresses that fine.  Note that
  check_for_isn_and_int8_passing_mismatch() exists because pg_upgrade has
  allowed source and destination clusters to differ in USE_FLOAT8_BYVAL.
 
 What we had was checking for float8_pass_by_value, but did nothing with
 it, so I assumed we had lost the check somehow.  I will remove detecting
 and checking of that value.  Thanks.

Sorry, I mean we didn't do anything with it in controldata.c, but I had
forgotten how we use it for isn, so I just added a C comment that there
is no need to check that it matches.  Thanks.

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

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pg_upgrade cleanup

2015-05-16 Thread Noah Misch
On Thu, May 14, 2015 at 10:06:11PM -0400, Bruce Momjian wrote:
 On Thu, May 14, 2015 at 09:56:53PM -0400, Bruce Momjian wrote:
  This patch makes pg_upgrade controldata checks more consistent, and adds
  a missing check for float8_pass_by_value.
 
 Sorry, I should have mentioned I applied this patch to head.  It isn't
 significant enough to backpatch.

A float8_pass_by_value match is unnecessary, and requiring it creates needless
hassle for users.  Switching between USE_FLOAT8_BYVAL binaries and
!USE_FLOAT8_BYVAL binaries requires an initdb to get different values in
pg_type.typbyval and pg_attribute.attbyval.  pg_upgrade's use of pg_dump to
migrate catalog content addresses that fine.  Note that
check_for_isn_and_int8_passing_mismatch() exists because pg_upgrade has
allowed source and destination clusters to differ in USE_FLOAT8_BYVAL.

The rest of this change is opaque to me.  More consistent with what?  The
old use of the tli variable sure looked like a bug, considering the variable
was never set to anything but zero.  What is the anticipated behavior change?


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] hstore_plpython regression test does not work on Python 3

2015-05-16 Thread Peter Eisentraut
On 5/16/15 12:06 PM, Tom Lane wrote:
 As exhibited for instance here:
 
 http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=spoonbilldt=2015-05-16%2011%3A00%3A07
 
 I've been able to replicate this on a Fedora 21 box: works fine with
 Python 2, fails with Python 3.  Seems like we still have an issue
 with reliance on a system-provided sort method.

Pushed a fix, tested with 2.3 .. 3.4.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] pg_audit documentation fixes

2015-05-16 Thread Peter Geoghegan
Attached patch makes minor tweaks to pg_audit docs.

-- 
Peter Geoghegan
diff --git a/doc/src/sgml/pgaudit.sgml b/doc/src/sgml/pgaudit.sgml
index b8df0d5..915b977 100644
--- a/doc/src/sgml/pgaudit.sgml
+++ b/doc/src/sgml/pgaudit.sgml
@@ -89,7 +89,7 @@ AUDIT: SESSION,33,2,DDL,CREATE TABLE,TABLE,public.important_table,CREATE TABLE i
   relation referenced in a statement.  No parsing is required to find all
   statements that touch a particular table.  In fact, the goal is that the
   statement text is provided primarily for deep forensics and should not be
-  the required for an audit.
+  required for an audit.
 /para
   /sect2
 
@@ -653,7 +653,7 @@ AUDIT: SESSION,36,1,DDL,ALTER TABLE,TABLE,public.test2,ALTER TABLE test RENAME T
 
   listitem
 para
-  Autovacuum and Autoanalyze are not logged, nor are they intended to be.
+  Autovacuum and Autoanalyze are not logged.
 /para
   /listitem
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support for N synchronous standby servers - take 2

2015-05-16 Thread Sawada Masahiko
On Fri, May 15, 2015 at 9:18 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Fri, May 15, 2015 at 8:55 PM, Beena Emerson memissemer...@gmail.com 
 wrote:
 There was a discussion on support for N synchronous standby servers started
 by Michael. Refer
 http://archives.postgresql.org/message-id/cab7npqr9c84ig0zuvhmqamq53vqsd4rc82vyci4dr27pvof...@mail.gmail.com
 . The use of hooks and dedicated language was suggested, however, it seemed
 to be an overkill for the scenario and there was no consensus on this.
 Exploring GUC-land was preferred.

 Cool.

 Please find attached a patch,  built on Michael's patch from above mentioned
 thread, which supports choosing different number of nodes from each set i.e.
 k nodes from set 1, l nodes from set 2, so on.
 The format of synchronous_standby_names has been updated to standby name
 followed by the required count separated by hyphen. Ex: 'aa-1, bb-3'.  The
 transaction waits for all the specified number of standby in each group. Any
 extra nodes with the same name will be considered potential. The special
 entry * for the standby name is also supported.

 I don't think that this is going in the good direction, what was
 suggested mainly by Robert was to use a micro-language that would
 allow far more extensibility that what you are proposing. See for
 example ca+tgmobpwoenmmepfx0jwrvqufxvbqrv26ezq_xhk21gxrx...@mail.gmail.com
 for some ideas. IMO, before writing any patch in this area we should
 find a clear consensus on what we want to do. Also, unrelated to this
 patch, we should really get first the patch implementing the... Hum...
 infrastructure for regression tests regarding replication and
 archiving to be able to have actual tests for this feature (working on
 it for next CF).

The dedicated language for multiple sync replication would be more
extensibility as you said, but I think there are not a lot of user who
want to or should use this.
IMHO such a dedicated extensible feature could be extension module,
i.g. contrib. And we could implement more simpler feature into
PostgreSQL core with some restriction.

Regards,

---
Sawada Masahiko


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] WALWriteLock contention

2015-05-16 Thread Robert Haas
On Fri, May 15, 2015 at 9:15 PM, Jeff Janes jeff.ja...@gmail.com wrote:
 I implemented this 2-3 years ago, just dropping the WALWriteLock immediately
 before the fsync and then picking it up again immediately after, and was
 surprised that I saw absolutely no improvement.  Of course it surely depends
 on the IO stack, but from what I saw it seemed that once a fsync landed in
 the kernel, any future ones on that file were blocked rather than
 consolidated.

Interesting.

 Alas I can't find the patch anymore, I can make more of an
 effort to dig it up if anyone cares.  Although it would probably be easier
 to reimplement it than it would be to find it and rebase it.

 I vaguely recall thinking that the post-fsync bookkeeping could be moved to
 a spin lock, with a fair bit of work, so that the WALWriteLock would not
 need to be picked up again, but the whole avenue didn't seem promising
 enough for me to worry about that part in detail.

 My goal there was to further improve group commit.  When running pgbench
 -j10 -c10, it was common to see fsyncs that alternated between flushing 1
 transaction, and 9 transactions. Because the first one to the gate would go
 through it and slam it on all the others, and it would take one fsync cycle
 for it reopen.

Hmm, yeah.  I remember somewhat (Peter Geoghegan, I think) mentioning
behavior like that before, but I had not made the connection to this
issue at that time.  This blog post is pretty depressing:

http://oldblog.antirez.com/post/fsync-different-thread-useless.html

It suggests that an fsync in progress blocks out not only other
fsyncs, but other writes to the same file, which for our purposes is
just awful.  More Googling around reveals that this is apparently
well-known to Linux kernel developers and that they don't seem excited
about fixing it.  :-(

crazy-ideaI wonder if we could write WAL to two different files in
alternation, so that we could be writing to one file which fsync-ing
the other./crazy-idea

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] upper planner path-ification

2015-05-16 Thread Robert Haas
On Wed, May 13, 2015 at 10:27 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 For the reasons I mentioned, I'd like to get to a point where
 subquery_planner's output is Paths not Plans as soon as possible.  But the
 idea of coarse representation of steps that we aren't trying to be smart
 about might be useful to save some labor in the short run.

 The zero-order version of that might be a single Path node type that
 represents do whatever grouping_planner would do, which we'd start to
 break down into multiple node types once we had the other APIs fixed.

So, getting back to this part, what's the value of returning a list of
Paths rather than a list of Plans?  It seems that, in general terms,
what we're trying to do is delay performing some of the work, so that
we can generate several candidates relatively inexpensively and decide
only later which one to turn into a Plan.  What's not entirely clear
to me is which parts of the work we're trying to delay.  For example,
target lists are normally computed when paths are converted to plans,
but for the higher-level plan nodes adding by grouping_planner, the
path list is typically just passed in.  So would the new path types be
expected to carry target lists of their own, or would they need to
figure out the target list on the fly at plan generation time?

One thing that seems like it might complicate things here is that a
lot of planner functions take PlannerInfo *root as an argument.  But
if we generate only paths in grouping_planner() and path-ify them
later, the subquery's root will not be available when we're trying to
do the Path - Plan transformation.  Presumably that means anything we
need from the PlannerInfo has to be dug out and saved in the path.

I think grouping_planner() is badly in need of some refactoring just
to make it shorter.  It's over 1000 lines of code, which IMHO is a
fairly ridiculous length for a single function.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] jsonb concatenate operator's semantics seem questionable

2015-05-16 Thread Peter Geoghegan
Another thing that I noticed about the new jsonb stuff is that the
concatenate operator is based on the hstore one. This works as
expected:

postgres=# select '{a:1}'::jsonb || '{a:2}';
?column?
--
 {a: 2}
(1 row)

However, the nesting doesn't match up -- containers are not merged
beyond the least-nested level:

postgres=# select '{a:{nested:1}}'::jsonb || '{a:{also nested:2}}';
 ?column?
---
 {a: {also nested: 2}}
(1 row)

This feels wrong to me. When jsonb was initially introduced, we took
inspiration for the *containment* (operator @ jsonb) semantics from
hstore, but since jsonb is nested it worked in a nested fashion. At
the top level and with no nested containers there was no real
difference, but we had to consider the behavior of more nested levels
carefully (the containment operator is clearly the most important
jsonb operator). I had envisaged that with the concatenation of jsonb,
concatenation would similarly behave in a nested fashion. Under this
scheme, the above query would perform nested concatenation as follows:

postgres=# select '{a:{nested:1}}'::jsonb || '{a:{also
nested:2}}'; -- does not match actual current behavior
 ?column?
---
 {a: {nested:1, also nested: 2}}
(1 row)

Now, I think it's good that the minus operator (operator - text and
friends) discussed on the nearby thread accepts a text (or int)
argument and remove string elements/pairs at the top level only. This
works exactly the same as existence (I happen to think that removing
elements/pairs at a nested level is likely to be more trouble than
it's worth, and so I don't really like the new jsonb - text[]
operator much, because it accepts a Postgres (not JSON) array of texts
that constitute a path, which feels odd). So I have no issue with at
least the plain minus operators' semantics. But I think that the
concatenate operator's current semantics are significantly less useful
than they could be, and are not consistent with the overall design of
jsonb.

I'm particularly concerned about a table containing many homogeneously
structured, deeply nested jsonb datums (think of the delicious URLs
dataset that jsonb was originally tested using for a good example of
that -- this is quite representative of how people use jsonb in the
real world). It would be almost impossible to perform insert-or-update
type operations to these deeply nested elements using hstore style
concatenation. You'd almost invariably end up removing a bunch of
irrelevant nested values of the documents, when you only intended to
update one deeply nested value.

Looking back at the discussion of the new jsonb stuff, a concern was
raised along these lines by Ilya Ashchepkov [1], but this was
dismissed. I feel pretty strongly that this should be revisited. I'm
willing to concede that we might not want to always merge containers
that are found in the same position during concatenation, but I think
it's more likely that we do. As with containment, my sense is that
there should be nothing special about the nesting level -- it should
not influence whether we merge rather than overwrite the operator's
lhs container (with or into the rhs container). Not everyone will
agree with this [2].

I'm sorry that I didn't get to this sooner, but I was rather busy when
it was being discussed.

[1] http://www.postgresql.org/message-id/55006879.2050...@dunslane.net
[2] http://www.postgresql.org/message-id/54ef61dd.7040...@agliodbs.com
-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Draft release notes up for review

2015-05-16 Thread Tom Lane
I've committed first-draft release notes for next week's back-branch
releases.  As usual, I've made a section for 9.4.2 that currently
includes items for all branches; I'll subdivide the items tomorrow.
If you wish to review, please send comments in the next 18 hours or so.

Patch is up now at
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=0563b4c0c3b7cc2323cfb63e11d723764e2d5f7d
and the formatted results should be visible on the devel docs build at
http://www.postgresql.org/docs/devel/static/
in a couple of hours.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Bug in jsonb minus operator

2015-05-16 Thread Peter Geoghegan
I'm seeing the following problem on the master branch:

postgres=# select '{foo:5}'::jsonb - 'bar'; -- okay
  ?column?

 {foo: 5}
(1 row)

postgres=# select '{foo:{bar:5}}'::jsonb - 'foo'; -- okay
 ?column?
--
 {}
(1 row)

postgres=# select '{foo:{bar:5}}'::jsonb - 'rion'; -- spurious error
ERROR:  XX000: unknown type of jsonb container to convert
LOCATION:  convertJsonbValue, jsonb_util.c:1430

This is an elog() - a can't happen error - due to this restriction
within convertJsonbValue():

/*
 * A JsonbValue passed as val should never have a type of jbvBinary, and
 * neither should any of its sub-components. Those values will be produced
 * by convertJsonbArray and convertJsonbObject, the results of which will
 * not be passed back to this function as an argument.
 */

This call to convertJsonbValue() actually originates from the final
line of the new jsonb_delete() function, here:

#5  0x00877e10 in jsonb_delete (fcinfo=0x160e060) at jsonfuncs.c:3389
3389 PG_RETURN_JSONB(JsonbValueToJsonb(res));

I explored writing a fix for this bug. I went back and forth on it,
but I think that the most promising approach might be to simply teach
convertJsonbValue() to care about jbvBinary-typed JsonbValue
variables. That way, the jsonb_delete() function could actually expect
this to work. I can't remember why I thought it was a good idea to
have convertJsonbValue() reject jbvBinary values, but I believe the
reason was that it simply wasn't necessary. As it says of conversion
from in memory to disk representation (at the top of json_util.c):

/*
 * Turn an in-memory JsonbValue into a Jsonb for on-disk storage.
 *
 * There isn't a JsonbToJsonbValue(), because generally we find it more
 * convenient to directly iterate through the Jsonb representation and only
 * really convert nested scalar values.  JsonbIteratorNext() does this, so that
 * clients of the iteration code don't have to directly deal with the binary
 * representation (JsonbDeepContains() is a notable exception, although all
 * exceptions are internal to this module).  In general, functions that accept
 * a JsonbValue argument are concerned with the manipulation of scalar values,
 * or simple containers of scalar values, where it would be inconvenient to
 * deal with a great amount of other state.
 */

jsonb_delete() is unusual in that it converts from a JsonbValue back
to the on-disk Jsonb varlena format, but with a nested jbvBinary-typed
value (in the presence of a nested object or array) -- it seems like
it wouldn't be too hard to just roll with that. jsonb_delete() makes
the mistake of not expecting to see jbvBinary values (since it doesn't
always recurse into the json structure). We shouldn't really deal with
jbvBinary-typed values specially from outside specialized utility
routines like JsonbDeepContains() as noted in the above comment,
though (so offhand I don't think we should teach jsonb_delete()
anything new, as that would be a modularity violation). Thoughts?

Note that the existence related operators (that, like the minus
operator should only work at the top level) don't go through
JsonbValue conversion of the lhs value as part of their search at all.
I don't think that their findJsonbValueFromContainer() routine (or a
similar routine) is the right way of approaching this, though - that's
a specialized routine, that doesn't care if an array value (which is,
you'll recall, a key for the purposes of existence checking) appears
once or multiple times. On that topic, I think it's sloppy that Table
9-41. Additional jsonb Operators isn't clear about the fact that the
operator - text op matches things on the same basis as the existence
operators -- notice how the existence operator notes with emphasis
that it cares about array *string* values only.

Finally, I don't think an operator implementation function that is
jsonb-only belongs anywhere other than jsonb_ops.c (for the same
reason, replacePath() should live in jsonb_util.c). And I'm
disappointed that the jsonb tests can no longer be run atomically with
'make installcheck TESTS=jsonb' - I liked being able to do that.
-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers