2015-09-14 14:11 GMT+02:00 Tomas Vondra <tomas.von...@2ndquadrant.com>:
> On 09/14/2015 01:15 PM, Shulgin, Oleksandr wrote:
>> On Mon, Sep 14, 2015 at 11:53 AM, Tomas Vondra
>> <tomas.von...@2ndquadrant.com <mailto:tomas.von...@2ndquadrant.com>>
>> On 09/14/2015 10:23 AM, Shulgin, Oleksandr wrote:
>> On Sat, Sep 12, 2015 at 11:50 AM, Tomas Vondra
>> <mailto:tomas.von...@2ndquadrant.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
>> NULL, but I guess the top of the current_query_stack might
>> 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
>> 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:
>> 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 */
>> /* 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.