Thread: Re: [HACKERS] strange behaviour on pooled alloc (fwd)

Vadim,

I wrote:
>     New version of AllocSet...() functions is committed. palloc()
>     is a macro now. The  memory  eating  problem  of  COPY  FROM,
>     INSERT ... SELECT and UPDATES on a table that has constraints
>     is fixed (new file nodes/freefuncs.c).
>
>     The settings in aset.c aren't optimal for  now,  because  the
>     settings in place force the portals_p2 test to fail (at least
>     here). Some informations for those who want to take a look at
>     it follow.
>
>     Reproducing the bug:
>         [...]
>
>     Guessings:
>         [...]

    The  guess  that some memory get's corrupted where right. The
    area affected is something else.

>     I'll keep on debugging, but  would  be  very  appreciated  if
>     someone could help.

    I love  gdb!  Wasn't  easy  to  find,  and  would  have  been
    impossible without such a nice, smart tool.

    The  problem was caused by QuerySnapshot beeing free()'d if a
    transaction uses many portals. In ExecutorStart(), the actual
    QuerySnapshot is placed into the execution state, and that is
    used subsequently for HeapTupleSatisfies() deep  down  during
    ExecutePlan().

    In  the  case  of  portals, every DECLARE CURSOR results in a
    call to  ExecutorStart().  The  corresponding  ExecutorEnd(),
    where  the  execution  state is thrown away, is delayed until
    CLOSE.

    The  tcop  calls  SetQuerySnapshot()  before  any   call   to
    ExecutorStart().    So   if   someone   opens  many  cursors,
    SetQuerySnapshot() free's snapshot data that  will  later  be
    needed when FETCH has to scan relations.

    I have modified ExecutorStart() so it makes it's private copy
    of the actual  QuerySnapshot  in  it's  own  executor  memory
    context. Could you please comment if what is in QuerySnapshot
    at the time of ExecutorStart() get's or should  get  modified
    anywhere  during  the  execution of a plan. The name snapshot
    tells me NOT. But you're the one to judge.

    Please have a look at it and comment.


Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#======================================== jwieck@debis.com (Jan Wieck) #

Jan Wieck wrote:
> 
>     I have modified ExecutorStart() so it makes it's private copy
>     of the actual  QuerySnapshot  in  it's  own  executor  memory
>     context. Could you please comment if what is in QuerySnapshot
>     at the time of ExecutorStart() get's or should  get  modified
>     anywhere  during  the  execution of a plan. The name snapshot
>     tells me NOT. But you're the one to judge.

You're correct. Alternativly, we could use some refcount
in Snapshot structure...

Vadim

>
> Jan Wieck wrote:
> >
> >     I have modified ExecutorStart() so it makes it's private copy
> >     of the actual  QuerySnapshot  in  it's  own  executor  memory
> >     context. Could you please comment if what is in QuerySnapshot
> >     at the time of ExecutorStart() get's or should  get  modified
> >     anywhere  during  the  execution of a plan. The name snapshot
> >     tells me NOT. But you're the one to judge.
>
> You're correct. Alternativly, we could use some refcount
> in Snapshot structure...

    As  it  is  now,  the snapshot data exists in the same memory
    context as the execution state which isn't free'd  explicitly
    (auto  done  on AllocSetReset() when memory context dies). No
    need to add another bookkeeping that's hard to track.

    So pooled memory allocation is done now.


Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#======================================== jwieck@debis.com (Jan Wieck) #