[MDEV-22028] Use ptr_arg in Field::store_*/Field::val_* Created: 2020-03-24  Updated: 2022-04-25

Status: Stalled
Project: MariaDB Server
Component/s: None
Fix Version/s: N/A

Type: Task Priority: Major
Reporter: Nikita Malyavin Assignee: Nikita Malyavin
Resolution: Unresolved Votes: 0
Labels: beginner-friendly


 Description   

This task could be splitted by two: one for store_* and one for val_*

We currently have following problem with Field class:

  • It has methods to store and load values with respect to the data type conversion required:
    store_* and val_* (e.g. Field::val_float(), Field::store_str(),...)
  • Those methods are virtual, and are overridden for certain ancestors to achieve a conversion to and from any fundamental type
  • The problem is: they use Field::ptr internally instead of an explicit argument
  • This leads to that in many places Field::ptr is changed to some value needed, then store_*/val_* ` is called, then it is changed back
    • in many places Field::move_field_offset() is used that. It could be potentially removed completely.

The task is:

  • Fix all store_* or val_* functions to accept uchar *ptr_arg explicitly. I think it's better to make a new name
    • in particular, we'll soon have Field_varstring::val_str_from_ptr for exactly that purpose
  • Eliminate hither-thither manipulations mentioned, across the code base, by locating Field::move_field_offset usages
  • Add a method that uses Field::ptr back, but make it non-virtual
    • e.g. 

      double Field::val_real() const 
      { return val_real_from_ptr(ptr); }

      and so on

 Comments   
Comment by Nikita Malyavin [ 2020-11-20 ]

nayuta-yanagisawa will work on this task

Comment by Nayuta Yanagisawa (Inactive) [ 2020-11-23 ]

Before writing code, I did a brief survey on the code base.

The following pattern frequently appears in the storage engine codes:

ptrdiff_t ptr_diff = buf - table->record[0];
 
field->move_field_offset(ptr_diff);
 
/* do something with field */
 
field->move_field_offset(-ptr_diff);

The pattern looks awful and could be easily misused, especially in an error-handling case. I agree that we should remove the pattern.

On the other hand, I also found some use cases, of move_field_offset(), which are not fit the above pattern and seem to be difficult to eliminate just by adding store_/val_ functions accepting *ptr_arg. For example see index_next_same() in handler.cc.

By the above observation, I'm considering to limit the scope of the issue to adding store_/val_ functions accepting *ptr_arg and removing move_field_offset() which can be simply replaced with the new functions.

Is the scope in line with your idea? @nikitamalyavin

Comment by Nayuta Yanagisawa (Inactive) [ 2020-11-23 ]

I don't know why I cannot mention other users but it is possibly an authority issue.

Comment by Nikita Malyavin [ 2020-11-23 ]

You are right, nayuta-yanagisawa. I was thinking a lot on the last days about it.

  • handler cases are untrivial as hell.
  • There are expressions on fields represented by Item_field. So if Item value calculation is involved beneath, we cannot remove move_field_offset usages, unless we refactor {{Item}}s.
  • We cannot refactor {{Item}}s that way – expressions may involve fields from several tables. Hence, the prototype as follows: 

    Item::val_int(uint *ptr);

    • is the wrong design for {{Item}}s. It will accept a pointer to one table only
Comment by Nikita Malyavin [ 2020-11-23 ]

So let's agree on leaving move_field_offset usages as is, at least not focus on them on this task.

Though I'd like to see a separate patch with several removals as a usage hint for other developers. We'll set up a trend with that, at least

move_field_offset is used a lot for a key buffer initializations (not our scope though) and for default/virtual value calculations. Check out write_record, fix_fields functions.

>I don't know why I cannot mention other users but it is possibly an authority issue.
try to use this syntax:

[~nikitamalyavin] -- Jira autocompletes it for me that way

Comment by Nayuta Yanagisawa (Inactive) [ 2020-11-23 ]

> So let's agree on leaving move_field_offset usages as is, at least not focus on them on this task.
> Though I'd like to see a separate patch with several removals as a usage hint for other developers. We'll set up a trend with that, at least

OK, I will work along with the above way.

> try to use this syntax:
Thanks. It seems to work. nikitamalyavin

Comment by Nayuta Yanagisawa (Inactive) [ 2020-12-12 ]

I'm working on it little by little. I will create a pull request on mariadb/server repository once I've replaced all val_** stuff. You can track my progress by the pull request, which is created just for sharing purposes, on my personal repository. https://github.com/nayuta-yanagisawa/server/pull/3/files

Generated at Thu Feb 08 09:11:39 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.