Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Index field_weight's value. #924

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

adam-vessey
Copy link

GitHub Issue: (link)

  • Other Relevant Links (Google Groups discussion, related pull requests,
    Release pull requests, etc.)

What does this Pull Request do?

Adds indexing in the DB for the value of the field_weight field, given it its intent is to sort things, which indexing can facilitate.

This DB indexing behaviour is not configurable via Drupal's GUI, and so gets into something of a weird place, essentially requiring code to manipulate it. Given this field is/was originally defined in islandora_core_feature, it seemed like the place to put this.

What's new?

Addition of index to for the field on install, and a post-update hook to start indexing it in existing installations.

  • Does this change add any new dependencies? No.
  • Does this change require any other modifications to be made to the repository
    (i.e. Regeneration activity, etc.)? No.
  • Could this change impact execution of existing code? Unlikely? Queries sorting of the weight should be faster?

How should this be tested?

  • Check for the presence of an index on the given field. On PostgreSQL, this is something like:
    drupal_default=> select indexname, indexdef from pg_indexes where tablename = 'node__field_weight';
    node__field_weight____pkey	CREATE UNIQUE INDEX node__field_weight____pkey ON public.node__field_weight USING btree (entity_id, deleted, delta, langcode)
    node__field_weight__bundle__idx	CREATE INDEX node__field_weight__bundle__idx ON public.node__field_weight USING btree (bundle)
    node__field_weight__revision_id__idx	CREATE INDEX node__field_weight__revision_id__idx ON public.node__field_weight USING btree (revision_id)
    drupal_default=>
    
    Analog on MySQL/MariaDB/whatever seems to be a query more like SHOW INDEX FROM node__field_weight;, but haven't tested such
  • Grab the updated code. Maybe clear cache?
  • Run Drupal's updates (drush updb or otherwise), it should find the post-update hook.
  • Check that the index now exists (note the node__field_weight__field_weight_value__idx bit, in the case of PostgreSQL):
    drupal_default=> select indexname, indexdef from pg_indexes where tablename = 'node__field_weight';
    [... the same three as listed above, but with the addition of:]
    node__field_weight__field_weight_value__idx	CREATE INDEX node__field_weight__field_weight_value__idx ON public.node__field_weight USING btree (field_weight_value)
    drupal_default=>
    

As for testing the installation side of things, not sure. Not sure there's any extant mechanism for performing the installation directly from the modules, without other configuration being imported which might interfere? I... suppose one might try installing the updated islandora_core_feature into a plain Drupal site, and see that the index gets populated?

Documentation Status

  • Does this change existing behaviour that's currently documented?
  • Does this change require new pages or sections of documentation?
  • Who does this need to be documented for?
  • Associated documentation pull request(s): ___ or documentation issue ___

Additional Notes:

Ironically, playing around with EXPLAIN ANALYZEing Drupal-generated queries in PostgreSQL, this index doesn't appear to be used. Other than other heuristics involving the size of the table, I suspect it is due to Drupal changing the ordering of NULL to be first, inline with MySQL/MariaDB/etc in queries, but indexes being left in their default ordering of NULL-last, and so to be incompatible with sorting operations.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/committers

@jordandukart
Copy link
Member

Does indeed create an index which should help makes thing a bit more speedy!

@wgilling
Copy link

somebody else needs to review before it can be merged.

@wgilling wgilling requested a review from rosiel January 18, 2023 18:13
@rosiel
Copy link
Member

rosiel commented Jan 18, 2023

Thank you for the testing instructions, they were great. SQL command was right on.

This PR indeed created the hook! Then, I added a bunch of pages, and re-ordered them. The order stuck, and was accurate in the "Children" list and the "OAI-PMH" views. So nothing bad that I could see. I admit I don't have enough time/resources/tools to conduct any analysis of whether this new index actually makes the views appear faster but from what I know about databases, it should (though probably not detectably unless you loaded a lot of pages out of order).

I'd say this is good to merge, but as of the call today, DGI's still doing internal QA. When you'd like me to hit the button, let me know.

@nchiasson-dgi
Copy link

Ran local tests using the provided PR. Appears to function as intended, appropriate tables are built and field indexed.

@rosiel You're good to hit the button!

@rosiel rosiel merged commit fe7e450 into Islandora:2.x Jan 25, 2023
@adam-vessey adam-vessey deleted the fix/index-field-weight branch January 25, 2023 18:07
rosiel pushed a commit to rosiel/islandora that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants