PG_FREE_IF_COPY in hash op

[Komzpa]

Mentally debugging memory consumption. PostGIS does free-if-copy dance here so why not also do it in H3?

[Komzpa]

@zachasme CI failed, and I'm not sure about failure.

[zachasme]

Hm, CI error doesn't seem related to your code. I think something is wrong with the pipeline. Let me figure it out. Thanks for the PR!

[Komzpa]

don't merge, segfaults:

2020-03-24 05:47:10.971 +03 [14695] LOG:  background worker "parallel worker" (PID 10582) was terminated by signal 11: Збой сэгмэнтацыі
2020-03-24 05:47:10.971 +03 [14695] DETAIL:  Failed process was running: create table osm_user_count_grid_h3 as (

[Komzpa]

Did not crash after this correction.

How do you feel about making H3 pass-by-value on 64bit platforms?

[Komzpa]

I've run tests manually, found another issue and now they pass locally.

[zachasme]

Thanks for all of the work, Kompza. 👍 I've fixed the CI checks, so rebasing onto master should make it pass.

This is the first time I'm seeing PG_FREE_IF_COPY, can you give me a quick explanation of the PR (is it fixing a memory problem?) or link to me some related documentation?

Regarding pass-by-value I went with pass-by-reference based on the Base Types in C-Language Functions documentation. What would a solution making H3 pass-by-value on 64bit platforms look like? Would it be a separate distribution or can we support both and fall back to pass-by-reference on non-64bit?

[Komzpa]

PG_FREE_IF_COPY is a macro that issues pfree if result of PG_GETARG_*_P happened to create a copy of initial pointer. If you look at PostGIS it's everywhere; I have found no reason why it should be omitted here. https://github.com/postgres/postgres/blob/master/src/include/fmgr.h#L246

What I see in postgres sources is that PG_GETARG_... is not returning a pointer but a value instead in smaller datatypes. In function local data it will either be optimized, or not that much of a copy. But such layout gives other possibility - to create variable byvalue-byreference types with compile time decision. For example, see https://github.com/postgres/postgres/blob/master/src/include/postgres.h#L700 - they're defining a compile time define and then later select either type-punning copier or follow-the-reference one. Type punning one compiles to zero assembly instructions.

[Komzpa]

H3 type itself can't be toasted, but my expectation is that if you create a new user defined type, its content can be toasted if it's long, and that's how h3index can get toasted and copied.

Course of action I propose:

• merge PG_FREE_IF_COPY in hash op #22.
• merge Sort Support routine #24.
• relayout code so that PG_GETARG_* is not the _P version that returns pointer, but a copying one just like in float8 code. That will kill the PG_FREE_IF_COPY things as it will be handled by function scope.
• allow switching between byval/byref as compile time defing.
• autodetect 64bit Datum platforms (maybe there's a Define in Postgres on that already) and select byval on them.

How about it?

[zachasme]

Thank you the explanation, that all sounds good to me 👍 If you rebase #24 on master I'll get that merged right away.

I'll get to work on the last three bullets.

Regarding point 3: What exactly do you mean by killing PG_FREE_IF_COPY after refactoring PG_GETARG from pointer to value? Should we remove them once the refactor is done, or should we make sure they are only called when compiled with pass-by-reference?

[Komzpa]

Rebased that thing in #24, hopefully well.

Point 3: the way things like float8 are treated in core is that they use non-_P version of macro. They are not using a pointer to float in code - they just use float.
https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/float.c#L729

This would make a typical thing in code like this:

/* Determines if the provided H3Index is a valid unidirectional edge index */
Datum
h3_unidirectional_edge_is_valid(PG_FUNCTION_ARGS)
{
	H3Index    edge = PG_GETARG_H3_INDEX(0);
	bool		isValid = h3UnidirectionalEdgeIsValid(edge);
	PG_RETURN_BOOL(isValid);
}

In this form up to your taste to make it this:

/* Determines if the provided H3Index is a valid unidirectional edge index */
Datum
h3_unidirectional_edge_is_valid(PG_FUNCTION_ARGS)
{
	PG_RETURN_BOOL(h3UnidirectionalEdgeIsValid(PG_GETARG_H3_INDEX(0)));
}

PG_GETARG_H3_INDEX has to be created. It should follow the footsteps of https://github.com/postgres/postgres/blob/master/src/include/postgres.h#L700.

Thanks a lot for getting to this :)

[zachasme]

That part was clear, the documentation and code you've sent have been very helpful.

What I meant regarding point 3 was whether this pull request will have to be reverted (removing PG_FREE_IF_COPY) after PG_GETARG_H3_INDEX has been implemented.