FOR PORTION OF memory leak checking domains

Hi Hackers,

I received an off-list memory leak report from Tomas Vondra. Details below:

> CREATE DOMAIN leakdom AS daterange CHECK (VALUE IS NULL OR NOT
> isempty(VALUE));
>
> CREATE TABLE leak (id int, valid_at leakdom, name text);
> --CREATE TABLE leak (id int, valid_at daterange, name text);
>
> INSERT INTO leak
> SELECT g, daterange('2000-01-01','2010-01-01'), 'x'
> FROM generate_series(1, 200_000) g;
>
> SET log_executor_stats = on;
>
> UPDATE leak FOR PORTION OF valid_at FROM '2004-01-01' TO '2005-01-01'
> SET name = 'y';
> ---
>
> With the domain, I get this:
>
> DETAIL: ! system usage stats:
> ! 6.003382 s user, 1.508554 s system, 7.633776 s elapsed
> ! [6.724448 s user, 1.533475 s system total]
> ! 2633248 kB max resident size
> ! 0/74240 [0/114192] filesystem blocks in/out
> ! 0/646011 [0/651587] page faults/reclaims, 0 [0] swaps
> ! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
> ! 13/309 [31/354] voluntary/involuntary context switches
>
> while with the plain daterange I get
>
> DETAIL: ! system usage stats:
> ! 3.857666 s user, 0.071827 s system, 4.046457 s elapsed
> ! [4.517879 s user, 0.114015 s system total]
> ! 79060 kB max resident size
> ! 0/92096 [0/149952] filesystem blocks in/out
> ! 0/7674 [0/14166] page faults/reclaims, 0 [0] swaps
> ! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
> ! 23/130 [39/159] voluntary/involuntary context switches
>
> That's ~2.6GB vs. ~80MB for the RSS, which seems like a lot. It can be
> made worse by using more rows in the table.
>
> AFAIK the issues is in nodeModifyTable.c, which does this:
>
> /*
> * Does the new Datum violate domain checks? Row-level CHECK
> * constraints are validated by ExecInsert, so we don't need to do
> * anything here for those.
> */
> if (forPortionOf->isDomain)
> domain_check(leftover, false, forPortionOf->rangeVar->vartype,
> NULL, NULL);
>
> where the NULLs mean it's running with CurrentMemoryContext, which is
> es_query_ctx. And hence the query-wide leak.
>
> domain_check() has a way to cache stuff once - that's what the extra
> argument is about. I don't know enough about this code, maybe it could
> be a local scratch space, but maybe it'd be better to add it to the FPO
> executor state:
>
> /* src/include/nodes/execnodes.h — ForPortionOfState */
> void *fp_domaininfo; /* cache space for domain_check() */
>
> and call it like this:
>
> if (forPortionOf->isDomain)
> domain_check(leftover, false, forPortionOf->rangeVar->vartype,
> &fpoState->fp_domaininfo, NULL);

I agree that is clearly a leak. His fix works and makes the memory
usage consistent between the domain and non-domain cases. My only
change to the above is to pass estate->es_query_cxt explicitly instead
of assuming it's already the CurrentMemoryContext. I also added a test
with a multi-row domain FOR PORTION OF update to validate that the
caching functions correctly.

Yours,

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com