Re: Patch: show relation and tuple infos of a lock to acquire

From: Christian Kruse <christian(at)2ndQuadrant(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Simon Riggs <simon(at)2ndQuadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Patch: show relation and tuple infos of a lock to acquire
Date: 2014-01-02 11:08:37
Message-ID: 20140102110837.GA17774@defunct.ch
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On 02/01/14 10:02, Andres Freund wrote:
> > Christian's idea of a context line seems plausible to me. I don't
> > care for this implementation too much --- a global variable? Ick.
>
> Yea, the data should be stored in ErrorContextCallback.arg instead.

Fixed.

I also palloc() the ErrorContextCallback itself. But it doesn't feel
right since I could not find a piece of code doing so. What do you
think?

> > Make a wrapper function for XactLockTableWait instead, please.
>
> I don't think that'd work out all too nicely - we do the
> XactLockTableWait() inside other functions like MultiXactIdWait(),
> passing all the detail along for those would end up being ugly. So using
> error context callbacks properly seems like the best way in the end.

Wrapping XactLockTableWait()/MultiXactIdWait() at least allows the
ErrorContextCallback and ErrorContextCallback.arg to live on the
stack. So what we could do is wrap this in a macro instead of a
function (like e.g. PG_TRY) or write two different functions. And I
don't like the two functions approach since it means duplication of
code.

While writing the response I really think writing a macro in PG_TRY
style for setting up and cleaning the error context callback I begin
to think that this would be the best way to go.

> > And I'm not real sure that showing the whole tuple contents is a good
> > thing; I can see people calling that a security leak, not to mention
> > that the performance consequences could be dire.
>
> I don't think that the security argument has too much merit given
> today's PG - we already log the full tuple for various constraint
> violations. And we also accept the performance penalty there. I don't
> see any easy way to select a sensible subset of columns to print here,
> and printing the columns is what would make investigating issues around
> this so much easier. At the very least we'd need to print the pkey
> columns and the columns of the unique key that might have been violated.

I agree. Even printing only the PK values would be much more
complex. As far as I can see we would even have to gain another lock
for this (see comment for relationHasPrimaryKey() in
src/backend/catalog/index.c).

Regards,

--
Christian Kruse http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachment Content-Type Size
show_table_name_and_tuple_in_lock_log_v2.patch text/x-diff 11.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2014-01-02 12:05:34 Re: [PATCH] Negative Transition Aggregate Functions (WIP)
Previous Message Andres Freund 2014-01-02 10:38:13 Re: [PATCH] Store Extension Options