Re: [Firebird-devel] ExprNode::FLAG_VALUE
On 17/01/2022 12:19, Dmitry Yemanov wrote: > > Why is it done this way? AFAIU, impureCount is known during the compile > time, so the whole impure area could be preallocated during the prepare > stage. > There are others similar patterns, see irsb_message for example. The size is know at compile time. To allocate directly, it would need a rpt structure or different impure offsets. This pattern is used with irsb_mrg_rpt. It's a bit uglier and it looks like it waste a bit (sizeof(Impure::irsb_mrg_repeat)) of memory here since it's already present one time in the struct: const size_t size = sizeof(struct Impure) + count * sizeof(Impure::irsb_mrg_repeat); Adriano Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] ExprNode::FLAG_VALUE
06.01.2022 17:13, Adriano dos Santos Fernandes wrote:
See also:
https://github.com/FirebirdSQL/firebird/issues/1355
https://github.com/FirebirdSQL/firebird/commit/626ab18c426fd32d482e02093e72e57330596174
Worth testing GROUP BY , ?
Since v3 we must be safe. Aggregate stream allocates impure space for
the expressions in its own region.
Correct, although I find it weird that the impure space is allocated
dynamically rather than statically (in the request's impure area):
unsigned impureCount = m_group ? m_group->getCount() : 0;
if (!impure->groupValues && impureCount > 0)
{
impure->groupValues = FB_NEW_POOL(*tdbb->getDefaultPool())
impure_value[impureCount];
memset(impure->groupValues, 0, sizeof(impure_value) * impureCount);
}
Why is it done this way? AFAIU, impureCount is known during the compile
time, so the whole impure area could be preallocated during the prepare
stage.
I'd say the flag exists because things were different before. Aggregate
were using expressions' impure regions.
Then FLAG_VALUE may be safely removed.
Dmitry
Firebird-Devel mailing list, web interface at
https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] ExprNode::FLAG_VALUE
On 06/01/2022 09:53, Dmitry Yemanov wrote:
> 06.01.2022 04:28, Adriano dos Santos Fernandes wrote:
>>
>> There is ExprNode::FLAG_VALUE ("Full value area required in impure
>> space"), inherited from old (2.5) code base nod_value.
>>
>> It's set by sort subsystem and used only for parameters and variables.
>
> Initially it as also used for fields. But at some point of time fields's
> impure area became always sizeof(impure_value_ex), unconditionally.
>
>> It makes then allocate impure space for impure_value_ex instead of
>> traditional dsc.
>
> I'd say allocating sizeof(dsc) is dangerous for these nodes. Look at
> EVL_assign_to(), for example. It does:
>
> impure_value* impure =
> (impure_value*) ((SCHAR *) request + node->nod_impure);
> ...
> impure->vlu_desc.dsc_dtype = desc->dsc_dtype;
> ...
> return >vlu_desc;
>
> i.e. it works just because impure_value has "dsc vlu_desc" at the first
> position and thus the used part of impure_value is equal to sizeof(dsc).
> Change that and the things get broken.
I'd say this code (looking at master) can be improved.
It's very weird for parameters. EVL_assign_to gets the descriptor inside
the ParameterNode. But multiple parameters usage in code results in
multiple ParameterNodes.
Instead of returning a dsc*, it seems better if EVL_assign_to accepts a
pointer/reference to a dsc, "allocated" as local variable in its callers.
This may works also for fields.
For variables, it already reassigns the impure variable and gets the
descriptor from the DeclareVariableNode.
> Try to use vlu_flags for
> arguments/variables (make them invariant, for example) and things get
> broken. Too fragile, IMO. I'd consider allocating sizeof(impure_value)
> instead of sizeof(dsc), just for safety sake.
>
FLAG_INVARIANT is marked by nodes for they own, so they must be
compatible with impure_value.
vlu_flags is used in the DeclareVariableNode for variables, so it looks ok.
Weird things happens in UdfCallNode.
And it's why it have this code (comment):
struct Impure
{
impure_value value; // must be first
Firebird::Array* temp;
};
>> Most nodes allocate space for impure_value. But not all of them.
>>
>> Literals directly return the descriptor set in compile time.
>>
>> I see no usage of the expressions impure_value in sort. And if they were
>> using, we'd certainly have a problem with literals.
>
> See also:
>
> https://github.com/FirebirdSQL/firebird/issues/1355
>
> https://github.com/FirebirdSQL/firebird/commit/626ab18c426fd32d482e02093e72e57330596174
>
>
> Worth testing GROUP BY , ?
>
Since v3 we must be safe. Aggregate stream allocates impure space for
the expressions in its own region.
I'd say the flag exists because things were different before. Aggregate
were using expressions' impure regions.
Adriano
Firebird-Devel mailing list, web interface at
https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] ExprNode::FLAG_VALUE
06.01.2022 04:28, Adriano dos Santos Fernandes wrote:
There is ExprNode::FLAG_VALUE ("Full value area required in impure
space"), inherited from old (2.5) code base nod_value.
It's set by sort subsystem and used only for parameters and variables.
Initially it as also used for fields. But at some point of time fields's
impure area became always sizeof(impure_value_ex), unconditionally.
It makes then allocate impure space for impure_value_ex instead of
traditional dsc.
I'd say allocating sizeof(dsc) is dangerous for these nodes. Look at
EVL_assign_to(), for example. It does:
impure_value* impure =
(impure_value*) ((SCHAR *) request + node->nod_impure);
...
impure->vlu_desc.dsc_dtype = desc->dsc_dtype;
...
return >vlu_desc;
i.e. it works just because impure_value has "dsc vlu_desc" at the first
position and thus the used part of impure_value is equal to sizeof(dsc).
Change that and the things get broken. Try to use vlu_flags for
arguments/variables (make them invariant, for example) and things get
broken. Too fragile, IMO. I'd consider allocating sizeof(impure_value)
instead of sizeof(dsc), just for safety sake.
Most nodes allocate space for impure_value. But not all of them.
Literals directly return the descriptor set in compile time.
I see no usage of the expressions impure_value in sort. And if they were
using, we'd certainly have a problem with literals.
See also:
https://github.com/FirebirdSQL/firebird/issues/1355
https://github.com/FirebirdSQL/firebird/commit/626ab18c426fd32d482e02093e72e57330596174
Worth testing GROUP BY , ?
Dmitry
Firebird-Devel mailing list, web interface at
https://lists.sourceforge.net/lists/listinfo/firebird-devel
[Firebird-devel] ExprNode::FLAG_VALUE
Hi!
There is ExprNode::FLAG_VALUE ("Full value area required in impure
space"), inherited from old (2.5) code base nod_value.
It's set by sort subsystem and used only for parameters and variables.
It makes then allocate impure space for impure_value_ex instead of
traditional dsc.
Most nodes allocate space for impure_value. But not all of them.
Literals directly return the descriptor set in compile time.
I see no usage of the expressions impure_value in sort. And if they were
using, we'd certainly have a problem with literals.
I see no need to have this flag.
Do anyone see something I'm not seeing?
Adriano
Firebird-Devel mailing list, web interface at
https://lists.sourceforge.net/lists/listinfo/firebird-devel
