[HACKERS] psql's \d and \dt are sending their complaints to different output files

[Oleksandr Shulgin]

Hello Hackers,

I wonder if it is intentional that \d complains on stderr if it cannot find
relations to match, but \dt prints the message to the current output file?

postgres=# \d xxx
Did not find any relation named "xxx".
postgres=# \dt xxx
No matching relations found.

I've noticed the difference exactly because my output was
(accidentally) redirected to a file and I didn't see the complaint from the
2nd backslash command.

By browsing and grepping src/bin/psql/describe.c I can see that
psql_error() (used in \d) is prevalent, as opposed to fprintf() to
pset.queryFout (used in \dt), but then it's a question if it should be
treated as an error or not.

I think can be helpful, though rarely, to be able to send the output of \d*
commands to a file.  At the same time it would be nice to see the message
on stderr instead of appending to the output file, in case the relation was
not found, because of less head-scratching needed to realize the problem.
So I'd vote for changing \dt (and alike) behavior to use stderr.

Regards,
--
Alex

Re: [HACKERS] proposal: psql statements \gstore \gstore_binary (instead COPY RAW)

[Oleksandr Shulgin]

On Dec 9, 2016 18:40, "Pavel Stehule"  wrote:

Hi

Long time I am pushing a COPY RAW - without success.

Now I propose functionally similar solution - reduced to only to psql
console

Now we have a statement \g for execution query, \gset for exec and store
result in memory and I propose \gstore for storing result in file and
\gstore_binary for storing result in file with binary passing. The query
result should be one row, one column.

Usage:

SELECT image FROM accounts WHERE id = xxx
\gstore_binary ~/image.png

What do you think about this proposal?


I might be missing something, but is it different from:

\t
\a
\o output_filename
SELECT ...
\o

?

--
Alex

Re: [HACKERS] Danger of automatic connection reset in psql

[Oleksandr Shulgin]

On Tue, Nov 22, 2016 at 5:28 AM, Pavel Stehule <pavel.steh...@gmail.com>
wrote:

>
> 2016-11-22 3:46 GMT+01:00 Robert Haas <robertmh...@gmail.com>:
>
>> On Mon, Nov 21, 2016 at 4:55 AM, Oleksandr Shulgin
>> <oleksandr.shul...@zalando.de> wrote:
>> > On Tue, Nov 15, 2016 at 4:10 PM, Jim Nasby <jim.na...@bluetreble.com>
>> wrote:
>> >>
>> >> On 11/14/16 5:41 AM, Oleksandr Shulgin wrote:
>> >>>
>> >>> Automatic connection reset is a nice feature for server development,
>> >>> IMO.  Is it really useful for anything else is a good question.
>> >>
>> >>
>> >> I use it all the time for application development; my rebuild script
>> will
>> >> forcibly kick everyone out to re-create the database. I put that in
>> because
>> >> I invariably end up with a random psql sitting somewhere that I don't
>> want
>> >> to track down.
>> >>
>> >> What currently stinks though is if the connection is dead and the next
>> >> command I run is a \i, psql just dies instead of re-connecting. It'd
>> be nice
>> >> if before reading the script it checked connection status and
>> attempted a
>> >> reconnect.
>> >>
>> >>> At least an option to control that behavior seems like a good idea,
>> >>> maybe even set it to 'no reconnect' by default, so that people who
>> >>> really use it can make conscious choice about enabling it in their
>> >>> .psqlrc or elsewhere.
>> >>
>> >>
>> >> +1, I don't think it needs to be the default.
>> >
>> >
>> > So if we go in this direction, should the option be specified from
>> command
>> > line or available via psqlrc (or both?)  I think both make sense.
>> >
>> > What should be the option and control variable names?  Something like:
>> > --reconnect and RECONNECT?  Should we allow reconnect in non-interactive
>> > mode?  I have no use case for that, but it might be different for
>> others.
>> > If non-interactive is not supported then it could be a simple boolean
>> > variable, otherwise we might want something like a tri-state: on / off /
>> > interactive (the last one being the default).
>>
>> I think it should just be another psql special variable, like
>> AUTOCOMMIT or VERBOSITY.  If the user wants to set it on the command
>> line, they can just use -v.  We don't need a separate, dedicated
>> option for this, I think.
>>
>
That makes sense to me.

In this case depends on a default. For almost all scripts the sensible
> value is "without reconnect". It be unfriendly to use this setting via -v
> variable.
>

Well, if you're running a script it should not be affected as long as
default value for this new variable is "interactive" or "off" (and you
didn't override it in psqlrc).  If you really want to get a "reconnect even
from the script" type of behavior, then you'll have to use -v or set the
variable from inside the script itself to "on".

--
Alex

Re: [HACKERS] Danger of automatic connection reset in psql

[Oleksandr Shulgin]

On Tue, Nov 15, 2016 at 4:10 PM, Jim Nasby <jim.na...@bluetreble.com> wrote:

> On 11/14/16 5:41 AM, Oleksandr Shulgin wrote:
>
>> Automatic connection reset is a nice feature for server development,
>> IMO.  Is it really useful for anything else is a good question.
>>
>
> I use it all the time for application development; my rebuild script will
> forcibly kick everyone out to re-create the database. I put that in because
> I invariably end up with a random psql sitting somewhere that I don't want
> to track down.
>
> What currently stinks though is if the connection is dead and the next
> command I run is a \i, psql just dies instead of re-connecting. It'd be
> nice if before reading the script it checked connection status and
> attempted a reconnect.
>
> At least an option to control that behavior seems like a good idea,
>> maybe even set it to 'no reconnect' by default, so that people who
>> really use it can make conscious choice about enabling it in their
>> .psqlrc or elsewhere.
>>
>
> +1, I don't think it needs to be the default.


So if we go in this direction, should the option be specified from command
line or available via psqlrc (or both?)  I think both make sense.

What should be the option and control variable names?  Something like:
--reconnect and RECONNECT?  Should we allow reconnect in non-interactive
mode?  I have no use case for that, but it might be different for others.
If non-interactive is not supported then it could be a simple boolean
variable, otherwise we might want something like a tri-state: on / off /
interactive (the last one being the default).

In any case it would make sense to rectify the difference in current
behavior when the failing command is \i somefile.sql.  It would be
appropriate to stop parsing the file and reset the connection.

Other thoughts?

--
Alex

Re: [HACKERS] Danger of automatic connection reset in psql

[Oleksandr Shulgin]

On Fri, Nov 11, 2016 at 5:37 AM, Pavel Stehule 
wrote:

>
> 2016-11-11 5:14 GMT+01:00 Ashutosh Bapat 
> :
>
>> >
>> > How about, instead of all this, adding an option to psql to suppress
>> > the automatic reconnect behavior?  When enabled, psql just exits
>> > instead of trying to reconnect.
>> >
>> +1. But, existing users may not notice addition of the new option and
>> still continue to face problem. If we add the option and make it
>> default not to reconnect, they will notice it and use option to get
>> older behaviour, but that will break applications relying on the
>> current behaviour. Either way, users will have at least something to
>> control the connection reset.
>>
>
> The reconnect in not interactive mode is a bad idea - and there should be
> disabled everywhere (it cannot to break any application - the behave of
> script when a server is restarted must be undefined (100% if ON_ERROR_STOP
> is active). In interactive mode it can be controlled by some psql session
> variables like AUTOCOMMIT.
>

Yes, I've never suggested it should affect non-interactive mode.

Automatic connection reset is a nice feature for server development, IMO.
Is it really useful for anything else is a good question.

At least an option to control that behavior seems like a good idea, maybe
even set it to 'no reconnect' by default, so that people who really use it
can make conscious choice about enabling it in their .psqlrc or elsewhere.

--
Alex

Re: [HACKERS] Danger of automatic connection reset in psql

[Oleksandr Shulgin]

On Mon, Nov 7, 2016 at 9:31 PM, Jim Nasby <jim.na...@bluetreble.com> wrote:

> On 11/4/16 4:04 AM, Oleksandr Shulgin wrote:
>
>> The psql process even exits with an error code 2, which might be not
>> that expected.  We could stop reading the file and reset connection
>> afterwards, but this is probably not that easy to achieve (think of
>> nested \i calls).
>>
>
> Well, if you stop reading from the file then I don't think more \i's will
> matter, no? I'd certainly like to see improvement here, because the
> difference in behavior with \i is annoying.
>

What I mean is you need a longjump out of the innermost \i back to the
toplevel interactive prompt.  This might be not a problem since this is
what already happens upon receiving SIGINT, I believe.

On the bigger question of how to better protect all these cases (cut &
> paste, etc), I think the only robust way to do that is for psql to track
> intended transaction status itself. With the amount of parsing it's already
> doing, maybe that wouldn't be that hard to add. It looks like this might
> require extra libpq calls to constantly check in on server status; I'm a
> bit surprised that result objects don't include that info.


This doesn't have to be solely about transaction status, though for
something immediately destructive such as DELETE or UPDATE one should
expect a transaction guard.  But for example, pasting something like the
following two lines

SET search_path = 'fancy_new_schema', 'old_boring_schema', public;
SELECT * FROM get_item_ids_to_delete(...);

can give slightly different results depending on whether the first
statement had it effect or not.  And since psql is trying to be very
helpful here by resetting the connection, it makes it all too easy to
overlook the problem.

What do you think about trying to read everything we can from the terminal
using non-blocking IO and only if that gives EWOULDBLOCK, starting the
interactive prompt?

--
Alex

Re: [HACKERS] Danger of automatic connection reset in psql

[Oleksandr Shulgin]

On Thu, Nov 3, 2016 at 12:26 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
> A couple of doubts/suggestions:
> 1. Should pset.conn_was_reset be set to false, when we make a
> connection in do_connection()? Usually pset.conn_was_reset would be
> initialised with 0 since it's a global variable, so this may not be
> necessary. But as a precaution should we do this?
>

Hi,

Thank you for your comments.

I think this is not necessary since do_connection() is also called from
MainLoop where we clear the flag explicitly.  I also don't see a way we
could enter do_connection() with the conn_was_reset flag set to true in the
first place.

It still makes sense to initialize it to false in startup.c, though.

2. Comment below should be updated to reflect the new change
> /* fall out of loop if lexer reached EOL */
> -   if (scan_result == PSCAN_INCOMPLETE ||
> +   if (pset.conn_was_reset ||
> +   scan_result == PSCAN_INCOMPLETE ||
>

Check.  See attached v2.

3. What happens when the connection is reset while the source is a
> file say specified by \i in an interactive shell?


In this case pset.cur_cmd_interactive is false (see mainloop.c:66) and we
don't attempt to reset a failed connection:

postgres(p)=# \i 1.sql
psql:1.sql:1: FATAL:  terminating connection due to administrator command
psql:1.sql:1: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
psql:1.sql:1: connection to server was lost
$

The psql process even exits with an error code 2, which might be not that
expected.  We could stop reading the file and reset connection afterwards,
but this is probably not that easy to achieve (think of nested \i calls).

I will still try to see if we can observe blocking status of a read on
stdin and if that might help in protecting from a more complex case with
pasting into terminal.

--
Alex
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
new file mode 100644
index a7789df..34a4507
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
*** CheckConnection(void)
*** 386,391 
--- 386,393 
  		}
  		else
  			psql_error("Succeeded.\n");
+ 
+ 		pset.conn_was_reset = true;
  	}
  
  	return OK;
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
new file mode 100644
index 37dfa4d..c049a39
*** a/src/bin/psql/mainloop.c
--- b/src/bin/psql/mainloop.c
*** MainLoop(FILE *source)
*** 390,401 
  	break;
  			}
  
! 			/* fall out of loop if lexer reached EOL */
  			if (scan_result == PSCAN_INCOMPLETE ||
! scan_result == PSCAN_EOL)
  break;
  		}
  
  		/* Add line to pending history if we didn't execute anything yet */
  		if (pset.cur_cmd_interactive && !line_saved_in_history)
  			pg_append_history(line, history_buf);
--- 390,404 
  	break;
  			}
  
! 			/* fall out of loop if lexer reached EOL or connection was reset */
  			if (scan_result == PSCAN_INCOMPLETE ||
! scan_result == PSCAN_EOL ||
! pset.conn_was_reset)
  break;
  		}
  
+ 		pset.conn_was_reset = false;
+ 
  		/* Add line to pending history if we didn't execute anything yet */
  		if (pset.cur_cmd_interactive && !line_saved_in_history)
  			pg_append_history(line, history_buf);
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
new file mode 100644
index 8cfe9d2..39a4be0
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
*** typedef struct _psqlSettings
*** 102,107 
--- 102,110 
  	FILE	   *cur_cmd_source; /* describe the status of the current main
   * loop */
  	bool		cur_cmd_interactive;
+ 	bool		conn_was_reset;	/* indicates that the connection was reset
+  * during the last attempt to execute an
+  * interactive command */
  	int			sversion;		/* backend server version */
  	const char *progname;		/* in case you renamed psql */
  	char	   *inputfile;		/* file being currently processed, if any */
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
new file mode 100644
index 7ce05fb..e238de9
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*** main(int argc, char *argv[])
*** 139,144 
--- 139,145 
  	pset.last_error_result = NULL;
  	pset.cur_cmd_source = stdin;
  	pset.cur_cmd_interactive = false;
+ 	pset.conn_was_reset = false;
  
  	/* We rely on unmentioned fields of pset.popt to start out 0/false/NULL */
  	pset.popt.topt.format = PRINT_ALIGNED;

-- 
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] Danger of automatic connection reset in psql

[Oleksandr Shulgin]

On Thu, Oct 20, 2016 at 12:28 PM, Oleksandr Shulgin <
oleksandr.shul...@zalando.de> wrote:

>
> Since this is already an improvement, I'm attaching a patch.
>
> If on the other hand, someone is pasting into psql's terminal a block of
> commands enclosed in BEGIN/COMMIT, the same bug is triggered: BEGIN doesn't
> have effect and the rest of commands run outside of transaction.
>
> Is it possible at all to protect against the latter case?  How?
>

One idea I was just suggested is to make interactive psql sessions buffer
in all available input, before it's going to block.  This way it doesn't
matter if the multiple statements are appearing on one line or are being
pasted.

Feasible?

--
Alex

Re: [HACKERS] Danger of automatic connection reset in psql

[Oleksandr Shulgin]

On Thu, Oct 20, 2016 at 12:28 PM, Oleksandr Shulgin <
oleksandr.shul...@zalando.de> wrote:
>
>
> I'm not the first one to discover that, a search in archives gives at
least 3 results:
>
>
https://www.postgresql.org/message-id/flat/1097174870.9273.8.camel%40ipso.snappymail.ca#1097174870.9273.8.ca...@ipso.snappymail.ca
> https://www.postgresql.org/message-id/flat/4BF6A496.2090106%40comgate.c

Sorry that one got broken, should be:

  https://www.postgresql.org/message-id/flat/4BF6A496.2090106%40comgate.cz

[HACKERS] Danger of automatic connection reset in psql

[Oleksandr Shulgin]

Hi Hackers!

When using psql interactively one might be tempted to guard potentially
destructive commands such as "UPDATE / DELETE / DROP " by starting
the input line with an explicit "BEGIN; ...".  This has the added benefit
that then you invoke the command by reverse-searching the command history,
you get it together with the guarding transaction open statement.

This, however, is not 100% safe as I've found out a few days ago.  Should
the connection to the server get lost, the first of semicolon-separated
statements, "BEGIN;" will only trigger connection reset, and if that is
successful the following command(s) are going to be executed on the newly
opened connection, but without the transaction guard.

I'm not the first one to discover that, a search in archives gives at least
3 results:

https://www.postgresql.org/message-id/flat/1097174870.9273.8.camel%40ipso.snappymail.ca#1097174870.9273.8.ca...@ipso.snappymail.ca
https://www.postgresql.org/message-id/flat/4BF6A496.2090106%40comgate.c
https://www.postgresql.org/message-id/flat/CAD3a31U%2BfSBsq%3Dtxw2G-D%2BfPND_UN-nSojrGyaD%2BhkYUzvxusQ%40mail.gmail.com

The second one even resulted in a TODO item:

  Prevent psql from sending remaining single-line multi-statement queries
  after reconnection

I was thinking that simply adding a bool flag in the pset struct, to
indicate that connection was reset during attempt to execute the last query
would do the trick, but it only helps in exactly the case described above.

Since this is already an improvement, I'm attaching a patch.

If on the other hand, someone is pasting into psql's terminal a block of
commands enclosed in BEGIN/COMMIT, the same bug is triggered: BEGIN doesn't
have effect and the rest of commands run outside of transaction.

Is it possible at all to protect against the latter case?  How?

--
Alex
From 3eae5e5d91084e0882a286bac464782701e17d21 Mon Sep 17 00:00:00 2001
From: Oleksandr Shulgin <oleksandr.shul...@zalando.de>
Date: Thu, 20 Oct 2016 12:24:48 +0200
Subject: [PATCH] psql: stop sending commands after connection reset

Previsouly an input line such as "BEGIN; UPDATE something..." could
result in UPDATE running outside of transaction if the first statement
happen to trigger connection reset.

NB: this does *not* protect from blocks of commands pasted into the
terminal.
---
 src/bin/psql/common.c   | 2 ++
 src/bin/psql/mainloop.c | 5 -
 src/bin/psql/settings.h | 3 +++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index a7789df..34a4507 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -386,6 +386,8 @@ CheckConnection(void)
 		}
 		else
 			psql_error("Succeeded.\n");
+
+		pset.conn_was_reset = true;
 	}
 
 	return OK;
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index 37dfa4d..6d39ce8 100644
--- a/src/bin/psql/mainloop.c
+++ b/src/bin/psql/mainloop.c
@@ -391,11 +391,14 @@ MainLoop(FILE *source)
 			}
 
 			/* fall out of loop if lexer reached EOL */
-			if (scan_result == PSCAN_INCOMPLETE ||
+			if (pset.conn_was_reset ||
+scan_result == PSCAN_INCOMPLETE ||
 scan_result == PSCAN_EOL)
 break;
 		}
 
+		pset.conn_was_reset = false;
+
 		/* Add line to pending history if we didn't execute anything yet */
 		if (pset.cur_cmd_interactive && !line_saved_in_history)
 			pg_append_history(line, history_buf);
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 8cfe9d2..39a4be0 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -102,6 +102,9 @@ typedef struct _psqlSettings
 	FILE	   *cur_cmd_source; /* describe the status of the current main
  * loop */
 	bool		cur_cmd_interactive;
+	bool		conn_was_reset;	/* indicates that the connection was reset
+ * during the last attempt to execute an
+ * interactive command */
 	int			sversion;		/* backend server version */
 	const char *progname;		/* in case you renamed psql */
 	char	   *inputfile;		/* file being currently processed, if any */
-- 
2.7.4


-- 
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] [WIP] Patches to enable extraction state of query execution from external session

[Oleksandr Shulgin]

On Tue, Aug 30, 2016 at 9:34 AM, Maksim Milyutin 
wrote:

> On Mon, Aug 29, 2016 at 5:22 PM, maksim > > wrote:
>>
>> Hi, hackers!
>>
>> Now I complete extension that provides facility to see the current
>> state of query execution working on external session in form of
>> EXPLAIN ANALYZE output. This extension works on 9.5 version, for 9.6
>> and later it doesn't support detailed statistics for parallel nodes
>> yet.
>>
>> I want to present patches to the latest version of PostgreSQL core
>> to enable this extension.
>>
>> Hello,
>>
>> Did you publish the extension itself yet?
>>
>>
> Hello, extension for version 9.5 is available in repository
> https://github.com/postgrespro/pg_query_state/tree/master.
>
> Last year (actually, exactly one year ago) I was trying to do something
>> very similar, and it quickly turned out that signals are not the best
>> way to make this sort of inspection.  You can find the discussion
>> here: https://www.postgresql.org/message-id/CACACo5Sz7G0MFauC082iM
>> =xx_hq7qq5ndr4jpo+h-o5vp6i...@mail.gmail.com
>>
>
> Thanks for link!
>
> My patch *custom_signal.patch* resolves the problem of «heavy» signal
> handlers. In essence, I follow the course offered in *procsignal.c* file.
> They define *ProcSignalReason* values on which the SUGUSR1 is multiplexed.
> Signal recent causes setting flags for *ProcessInterrupt* actuating, i.e.
> procsignal_sigusr1_handler() only sets specific flags. When
> CHECK_FOR_INTERRUPTS appears later on query execution *ProcessInterrupt* is
> called. Then triggered user defined signal handler is executed.
> As a result, we have a deferred signal handling.
>

Yes, but the problem is that nothing gives you the guarantee that at the
moment you decide to handle the interrupt, the QueryDesc structures you're
looking at are in a good shape for Explain* functions to run on them.  Even
if that appears to be the case in your testing now, there's no way to tell
if that will be the case at any future point in time.

Another problem is use if shm_mq facility, because it manipulates the state
of process latch.  This is not supposed to happen to a backend happily
performing its tasks, at random due to external factors, and this is what
the proposed approach introduces.

--
Alex

Re: [HACKERS] [WIP] Patches to enable extraction state of query execution from external session

[Oleksandr Shulgin]

On Mon, Aug 29, 2016 at 5:22 PM, maksim  wrote:

> Hi, hackers!
>
> Now I complete extension that provides facility to see the current state
> of query execution working on external session in form of EXPLAIN ANALYZE
> output. This extension works on 9.5 version, for 9.6 and later it doesn't
> support detailed statistics for parallel nodes yet.
>
> I want to present patches to the latest version of PostgreSQL core to
> enable this extension.
>
Hello,

Did you publish the extension itself yet?

Last year (actually, exactly one year ago) I was trying to do something
very similar, and it quickly turned out that signals are not the best way
to make this sort of inspection.  You can find the discussion here:
https://www.postgresql.org/message-id/CACACo5Sz7G0MFauC082iM=xx_hq7qq5ndr4jpo+h-o5vp6i...@mail.gmail.com

Regards.
--
Alex

Re: [HACKERS] Stopping logical replication protocol

[Oleksandr Shulgin]

On Fri, May 6, 2016 at 5:23 PM, Vladimir Gordiychuk 
wrote:

> Hi all,
>
> During implementing logical replication protocol for pgjdbc
> https://github.com/pgjdbc/pgjdbc/pull/550 I faced with strange behavior
> of the *walcender.c*:
>
>1. When WAL consumer catchup master and change his state to streaming,
>not available normally complete replication by send CopyDone message until
>will not generate/create new WAL record. It occurs because logical decoding
>located in *WalSndWaitForWal* until will not available next WAL
>record, and it method receive message from consumer even reply on CopyDone
>with CopyDone but ignore exit from loop and we can wait many times waiting
>CommandStatus & ReadyForQuery packages on consumer.
>2. Logical decoding ignore message from consumer during decoding and
>writing transaction in socket(*WalSndWriteData*). It affect long
>transactions with many changes. Because for example if we start decoding
>transaction that insert 1 million records and after consume 1% of it date
>we
>decide stop replication, it will be not available until whole million
>record will not send to consumer.
>
> How exactly are you stopping the replication?  If you just stop reading
you'll likely to hit some problems, but what if you also close the
connection?  I don't think there is any other supported way to do it.

I was working last year on adding support for replication protocol to the
Python driver: https://github.com/psycopg/psycopg2/pull/322   It would be
nice if you could skim through this implementation, since it didn't receive
a whole lot of review.

Cheers.
--
Alex

Re: [HACKERS] Add jsonb_compact(...) for whitespace-free jsonb to text

[Oleksandr Shulgin]

On Mon, May 2, 2016 at 4:04 PM, Andrew Dunstan  wrote:

>
> On 05/02/2016 04:56 AM, Shulgin, Oleksandr wrote:
>
>> On Sun, May 1, 2016 at 3:22 AM, Andrew Dunstan > > wrote:
>>
>> On 04/29/2016 06:11 PM, Merlin Moncure wrote:
>>
>> This is a simple matter of removing spaces in the occasional C
>> string
>> literal in the serialization routines and adding a json_pretty
>> function.
>>
>>
>> I spent a few hours on this. See
>> 
>> for WIP - there are three commits. No regression tests yet for the
>> two new functions (json_squash and json_pretty), Otherwise fairly
>> complete. Removing whitespace generation was pretty simple for
>> both json and jsonb.
>>
>>
>> Looks good, thank you!
>>
>> It would make sense IMO to rename FormatState's `indent' field as
>> `pretty': it's being used to add whitespace between the punctuation, not
>> only at start of a line.  I'd also move the "if (indent)" check out of
>> add_indent(): just don't call it if no indent is needed.
>>
>> I'll try to play with the patch to produce some regression tests for the
>> new functions.
>>
>>
> It was done the way it was to be as consistent as possible with how it's
> done for jsonb (c.f. jsonb.c:JsonbToCStringWorker and jsonb.c::add_indent).
>

Ah, I see.

Simply taking regression tests for jsonb_pretty() and using them against
json_pretty() revealed a bug with extra indent being added before every
array/object start.  Attached patch fixes that and adds the regression
tests.

For json_squash() I've taken the same three test values, for lack of a
better idea at the moment.  At least we are testing key order stability and
that no whitespace is spit out.

Regards,
--
Alex
diff --git a/src/backend/utils/adt/jsonfuncs.c 
b/src/backend/utils/adt/jsonfuncs.c
new file mode 100644
index d82c9c8..77d8325
*** a/src/backend/utils/adt/jsonfuncs.c
--- b/src/backend/utils/adt/jsonfuncs.c
*** fmt_object_field_start(void *state, char
*** 3394,3402 
  
escape_json(_state->strval, fname);
  
!   appendStringInfoCharMacro(_state->strval, ':');
!   if (_state->indent)
!   appendStringInfoCharMacro(_state->strval, ' ');
  
_state->last_was_key = true;
  }
--- 3394,3400 
  
escape_json(_state->strval, fname);
  
!   appendBinaryStringInfo(_state->strval, ": ", _state->indent ? 2 : 1);
  
_state->last_was_key = true;
  }
*** fmt_array_element_start(void *state, boo
*** 3409,3417 
if (!_state->first)
appendStringInfoCharMacro(_state->strval, ',');
_state->first = false;
! 
!   add_indent(_state->strval, _state->indent, _state->lex->lex_level);
! 
  }
  
  static void
--- 3407,3413 
if (!_state->first)
appendStringInfoCharMacro(_state->strval, ',');
_state->first = false;
!   _state->last_was_key = false;
  }
  
  static void
*** fmt_scalar(void *state, char *token, Jso
*** 3419,3424 
--- 3415,3424 
  {
FormatState *_state = (FormatState *) state;
  
+   if (_state->lex->lex_level > 0)
+   add_indent(_state->strval, _state->indent && 
!_state->last_was_key,
+  _state->lex->lex_level);
+ 
if (tokentype == JSON_TOKEN_STRING)
escape_json(_state->strval, token);
else
diff --git a/src/test/regress/expected/json.out 
b/src/test/regress/expected/json.out
new file mode 100644
index 0c45c64..7af4022
*** a/src/test/regress/expected/json.out
--- b/src/test/regress/expected/json.out
*** select json_strip_nulls('{"a": {"b": nul
*** 1658,1660 
--- 1658,1736 
   {"a":{},"d":{}}
  (1 row)
  
+ select json_pretty('{"a": "test", "b": [1, 2, 3], "c": "test3", "d":{"dd": 
"test4", "dd2":{"ddd": "test5"}}}');
+ json_pretty 
+ 
+  { +
+  "a": "test",  +
+  "b": [+
+  1,+
+  2,+
+  3 +
+  ],+
+  "c": "test3", +
+  "d": {+
+  "dd": "test4",+
+  "dd2": {  +
+  "ddd": "test5"+
+  } +
+  } +
+  }
+ (1 row)
+ 
+ select json_pretty('[{"f1":1,"f2":null},2,null,[[{"x":true},6,7],8],3]');
+ json_pretty
+ ---
+  [+
+  {+
+  "f1": 1, +
+  "f2": null   +
+  },   +
+  2,   +
+  null,+
+  [+
+  [+
+  {+
+  "x": true+
+  },   +
+ 

Re: [HACKERS] Replication connection URI?

[Oleksandr Shulgin]

Heikki Linnakangas  writes:

> On 11/25/2014 05:11 PM, Heikki Linnakangas wrote:
>> On 11/24/2014 06:05 PM, Alex Shulgin wrote:
>>> Heikki Linnakangas  writes:
>
> It appears that replication connection doesn't support URI but only the
> traditional conninfo string.
>
> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in 
> libpqrcv_connect():
>
>snprintf(conninfo_repl, sizeof(conninfo_repl),
> "%s dbname=replication replication=true 
> fallback_application_name=walreceiver",
> conninfo);
>
> A patch to fix this welcome?

 Yeah, seems like an oversight. Hopefully you can fix that without
 teaching libpqwalreceiver what connection URIs look like..
>>>
>>> Please see attached.  We're lucky that PQconnectdbParams has an option
>>> to parse and expand the first dbname parameter if it looks like a
>>> connection string (or a URI).
>>>
>>> The first patch is not on topic, I just spotted this missing check.
>>>
>>> The second one is a self-contained fix, but the third one which is the
>>> actual patch depends on the second one, because it specifies the dbname
>>> keyword two times: first to parse the conninfo/URI, then to override any
>>> dbname provided by the user with "replication" pseudo-database name.
>>
>> Hmm. Should we backpatch the second patch? It sure seems like an
>> oversight rather than deliberate that you can't override dbname from the
>> connection string with a later dbname keyword. I'd say "yes".
>>
>> How about the third patch? Probably not; it was an oversight with the
>> connection URI patch that it could not be used in primary_conninfo, but
>> it's not a big deal in practice as you can always use a non-URI
>> connection string instead.
>
> Ok, committed the second patch to all stable branches, and the third 
> patch to master.

It still looks like a bug that primary_conninfo doesn't accept URIs,
even though they were supposed to be handled transparently by all
interfaces using libpq...

Any chance we reconsider and back-patch this up to 9.2?

--
Alex


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