search

hydradatabase / hydra

Hydra: Column-oriented Postgres. Add scalable analytics to your project in minutes.
https://www.hydra.so
Apache License 2.0
2.83k stars 76 forks source link

Remove redundant code in ColumnarStorageUpdateIfNeeded #249

Closed japinli closed 7 months ago

japinli commented 7 months ago

The ColumnarStorageIsCurrent will attempt to open SMGR when it doesn't open, so ColumnarStorageUpdateIfNeeded is not required to check SMGR.

OTOH, ColumnarStorageIsCurrent only returns true if metapage exists and is the current version, so fix the comment.

wuputah commented 7 months ago

I realized I can't approve running the Github Actions CI due the action never being triggered in the first place, due to how our if statements work in our Github Actions. Something to look into…

JerrySievert commented 7 months ago

not necessarily redundant. smgrsetowner can fail silently when assertions are not enabled, so in the actual unlikely event that this fails, the unlikely call should catch it.

japinli commented 7 months ago

not necessarily redundant. smgrsetowner can fail silently when assertions are not enabled, so in the actual unlikely event that this fails, the unlikely call should catch it.

I cannot get your mind. Since ColumnarStorageIsCurrent() has already checked this using unlikely(), so if there is a fail, ColumnarStorageIsCurrent() will catch it.

JerrySievert commented 7 months ago

lines 376-380 of columnar_storage.c call smgrsetowner which can silently fail to set an owner when assertions in Postgres are disabled (assertions are generally only enabled for development, not for distributed releases). this leaves rel->rd_smgr as NULL, and returns the results of ColumnarMetapageIsCurrent.

japinli commented 7 months ago

lines 376-380 of columnar_storage.c call smgrsetowner which can silently fail to set an owner when assertions in Postgres are disabled (assertions are generally only enabled for development, not for distributed releases). this leaves rel->rd_smgr as NULL, and returns the results of ColumnarMetapageIsCurrent.

Thanks for your patient explanation. Both ColumnarMetapageIsCurrent and ColumnarStorageUpdateIfNeeded have the same check and set logic for rel->rd_smgr. If ColumnarMetapageIsCurrent leaves rel->rd_smgr as NULL, how does ColumnarStorageUpdateIfNeeded make it non-NULL? I'm feeling a bit confused about this.