Re: On-demand running query plans using auto_explain and signals

2015-09-14 14:11 GMT+02:00 Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>:

>
>
> On 09/14/2015 01:15 PM, Shulgin, Oleksandr wrote:
>
>> On Mon, Sep 14, 2015 at 11:53 AM, Tomas Vondra
>> <tomas(dot)vondra(at)2ndquadrant(dot)com <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>>
>> wrote:
>>
>>
>> On 09/14/2015 10:23 AM, Shulgin, Oleksandr wrote:
>>
>> On Sat, Sep 12, 2015 at 11:50 AM, Tomas Vondra
>> <tomas(dot)vondra(at)2ndquadrant(dot)com
>> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>
>> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com
>>
>> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>>> wrote:
>>
>> ...
>>
>> - Attempts to get plan for simple insert queries like this
>>
>> INSERT INTO x SELECT * FROM x;
>>
>> end with a segfault, because ActivePortal->queryDesc is
>> 0x0 for this
>> query. Needs investigation.
>>
>>
>> Yes, I've hit the same problem after submitting the latest
>> version of
>> the patch. For now I've just added a check for queryDesc being
>> not
>> NULL, but I guess the top of the current_query_stack might
>> contains
>> something useful. Something I need to try.
>>
>>
>> Well, the thing is we're able to do EXPLAIN on those queries, and
>> IIRC auto_explain can log them too. So perhaps look into the hooks
>> where they take the queryDesc in those cases - it has to be
>> available somewhere.
>>
>>
>> Yes, this seems to work. I was able to successfully query "create table
>> as" and even "explain analyze" for the plans. In both cases
>> ActivePortal doesn't have a valid queryDesc, but the executor hook
>> captures one.
>>
>> - The lockless approach seems fine to me, although I think
>> the fear
>> of performance issues is a bit moot (I don't think we
>> expect large
>> number of processes calling pg_cmdstatus at the same
>> time). But
>> it's not significantly more complex, so why not.
>>
>>
>> I believe the main benefit of the less-locking approach is that if
>> something goes wrong when two backends tried to communicate it
>> doesn't
>> prevent the rest of them from operating, because there is no
>> shared (and
>> thus locked) communication channel.
>>
>>
>> Sure, but I think it really deserves a bit more detailed explanation
>> of the protocol, and discussion of the expected behavior for some
>> basic failure types.
>>
>> For example - what happens when a backend requests info, but dies
>> before receiving it, and the backed is reused for another
>> connection? Doesn't this introduce a race condition? Perhaps not,
>> I'm just asking.
>>
>>
>> Now explained better in code comments.
>>
>> The worst thing that could happen in this case I believe is that the
>> requesting backend will not receive any response from the second use of
>> its slot due to the slot being marked as processed by the backend that
>> was asked first:
>>
>> A:
>> slot->is_processed = false;
>> slot->is_valid = true;
>> /* signals backend B */;
>> shm_mq_read(); /* blocks */
>>
>> B: /* finds the slot and prepares the response */
>>
>> A: /* decides to bail out */
>> InvalidateCmdStatusSlot();
>> shm_mq_detach();
>> /* exits pg_cmdstatus() */
>>
>> /* pg_cmdstatus() is called again */
>> /* initializes the slot and signals some backend */
>> shm_mq_read(); /* blocks */
>>
>> B: /* completes preparing the response */
>> slot->is_processed = true;
>> shm_mq_send() => SHM_MQ_DETACHED
>> slot->is_valid = false;
>> /* gives up with this slot */
>>
>
hmm .. yes - there have to be check if slot is related still to queue
before to change is_valid attribute.

Regards

Pavel