remove old attribute before inserting renamed attribute

[kmuehlbauer]

Proof of #1388

Update: added possible fix.

This changes the order of processing. The old attribute is removed from dense storage before the renamed attribute is inserted. With the former behavior, deleting a renamed attribute wasn't possible any more.

Questions:

Why is removing/inserting from/in dense storage needed anyway? Wouldn't just updating the indexes with the new name be sufficient? I'm sure I'm missing background information on the hdf5 internals, so any pointers are welcome.

[kmuehlbauer]

@byrnHDF Would you mind approving the workflow? I just want to see if and how things fail here. At least we would have some traceback to better understand what might be the cause. Thanks.

[kmuehlbauer]

I've added a fix which solves the renaming/delete issue. At least it works locally on my machine. Please consider approving the workflow.

[takluyver]

(I think HDF group are in Illinois, so it will probably be a few more hours before they're online to approve the Github workflow ;-)

[kmuehlbauer]

(I think HDF group are in Illinois, so it will probably be a few more hours before they're online to approve the Github workflow ;-)

Thanks @takluyver, I'll wait patiently now. 😁

[kmuehlbauer]

Thanks for letting the workflow run. I've fixed the linter errors (hopefully). Should now be ready another try and for review. Please let me know, if and what I can do to finalize this. Thanks!

[epourmal]

Thanks for letting the workflow run. I've fixed the linter errors (hopefully). Should now be ready another try and for review. Please let me know, if and what I can do to finalize this. Thanks!

We need to check what the library behavior should be when creation order of attributes is tracked and then an attribute is deleted. There may be a deeper issue here and the change like this may affect netCDF-4 apps. Stay tuned.

[kmuehlbauer]

@epourmal No worries, I'm interested to solve this in the best possible way.

Actually I'm maintaining h5netcdf, a Python library which writes netcdf4 files via h5py. So anything which works for netcdf-c should work for h5py/h5netcdf too.

And even if my proposed fix is not the correct tool, we might find the correct one in the process.

[vchoi-hdfgroup]

The suggested fix to swap the order of processing in H5A__dense_rename() to:
--delete old attribute first from dense storage via H5A__dense_remove()
--then insert renamed attribute into dense storage via H5A__dense_insert()

I think we need to keep the existing order of processing (insert first then delete), see comments for H5O__attr_link() which is called between the above two routines:
-- /* Increment reference count on attribute components /
-- / (so that they aren't deleted when the attribute is removed shortly) */

The actual problem for not able to delete a renamed attribute is because:
--The attribute’s creation order got removed from the creation order index b-tree in H5A__dense_remove()

Need to work up a better fix.
My input is:
Since the ‘rename’ operation just changes the name index b-tree and the creation order index btree for that attribute remains intact, my input for the suggested fix is:
--Don’t insert/remove the attribute’s creation order from the creation order index b-tree for the ‘rename’ operation in H5A__dense_insert()/H5A__dense_remove()

[vchoi-hdfgroup]

Sorry I closed the PR by accident.

For the question: "Why is removing/inserting from/in dense storage needed anyway? Wouldn't just updating the indexes with the new name be sufficient? "
‘Rename’ is changing the name of the object, therefore changing its position in the name index b-tree.

[kmuehlbauer]

@vchoi-hdfgroup Thanks for the details and suggestions.

So would it be possible to not use H5A__dense_insert()/H5A__dense_remove() at all and just copy the necessary lines of code into H5A__dense_rename() at the appropriate places?

My C skills are quite rusty. I can try to come up with a solution, but if anyone from inside hdf5 group immediately sees a solution, I'd very much appreciate overriding my PR.

This quite aged bug should be resolved soonish.

[epourmal]

@kmuehlbauer,

We will not have anyone right now to work on the issue. Please, go ahead and try implement what was suggested and submit new PR; mark it with "work in progress" (WIP) and we will be continue working with you. Don't worry about your C skills :-)

[kmuehlbauer]

I'm sorry, but I'll not have time and energy to work on this PR. I've updated my issue #1388 with more details and would appreciate if this could be solved by HDF5. Thanks for the support!