Date: Thu, 12 Dec 2019 20:19:01 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Alexey Bashtanov <bashtanov@imap.cc>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: log bind parameter values on error
Message-ID: <20191212231901.GA24177@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20191212223252.pphhrzusu2rxmm3c@alap3.anarazel.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hello

On 2019-Dec-12, Andres Freund wrote:

> I see that you pushed this patch.

Yeah, a version of it -- significantly different from what Alexey
submitted last.

> I'm unfortunately unhappy with the
> approach taken.  As previously said, handling a lot of this from exec_*
> is a mistake in my opinion. Pretty much the same problem exists for
> parametrized query execution from other contexts, e.g. for queries
> executed inside plpgsql. By putting the responsibility to manage error
> contexts etc from with exec_*, we'd need to add a copy of that to pretty
> much every place executing queries. This should all be in the portal
> code, with an optimization for computing the parameter values from
> strings inside postgres.c, when the input is text.

Hmm.  I think there are two pieces of interest for this feature.  One
saves a text version of the params; the other is setting the error
context that reports them.  I agree that possibly it would work to have
PortalStart be in charge of the former.

However, AFAICS the error context must be set up in each place that's
executing the query; I don't think it would work to try to hack it in
SPI_cursor_open_internal, for example which is what plpgsql uses; rather
it has to be in plpgsql/pl_exec.c itself.  (For example, you cannot even
put it directly in exec_dynquery_with_params -- it has to be in the
callers of that routine, and surround all code until after
exec_for_query.)

I think the current state of affairs is already a good improvement over
what was there before.  Can it be improved?  Sure.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services